Skip to content

Return noop instrument if name is invalid#4383

Merged
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:instrument-name
Apr 20, 2022
Merged

Return noop instrument if name is invalid#4383
jack-berg merged 5 commits intoopen-telemetry:mainfrom
jack-berg:instrument-name

Conversation

@jack-berg
Copy link
Copy Markdown
Member

@jack-berg jack-berg commented Apr 14, 2022

Resolves #4378.

I think returning a noop instrument is the least wrong solution:

  • Throwing an exception is not in line with API patterns
  • If we log a warning but allowed the invalid name to be used, we'd put the burden on exporters to figure out how to handle metrics that break the rules.
  • If we log a warning an transform the name to be valid, we'd be making up unspecified rules, and could cause unexpected conflicts with other metrics.

@jack-berg jack-berg requested a review from a user April 14, 2022 21:53
@jack-berg jack-berg requested a review from Oberon00 as a code owner April 14, 2022 21:53
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 14, 2022 •
edited
Loading

Codecov Report

Merging #4383 (5bfd124) into main (c2d8f6a) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 5bfd124 differs from pull request most recent head 31d80b0. Consider uploading reports for the commit 31d80b0 to get more accurate results

@@             Coverage Diff              @@
##               main    #4383      +/-   ##
============================================
+ Coverage     89.94%   90.00%   +0.05%     
- Complexity     4908     4920      +12     
============================================
  Files           567      567              
  Lines         15199    15219      +20     
  Branches       1465     1465              
============================================
+ Hits          13671    13698      +27     
+ Misses         1058     1051       -7     
  Partials        470      470
Impacted Files Coverage Δ
.../io/opentelemetry/api/internal/ValidationUtil.java 100.00% <100.00%> (ø)
...ava/io/opentelemetry/api/metrics/DefaultMeter.java 100.00% <100.00%> (ø)
...in/java/io/opentelemetry/sdk/metrics/SdkMeter.java 100.00% <100.00%> (ø)
...emetry/api/baggage/propagation/PercentEscaper.java 84.21% <0.00%> (+4.51%) ⬆️
...ava/io/opentelemetry/sdk/internal/RateLimiter.java 100.00% <0.00%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d8f6a...31d80b0. Read the comment docs.

jkwatson
jkwatson reviewed Apr 14, 2022
View reviewed changes
Comment thread sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeter.java Outdated
anuraaga
anuraaga reviewed Apr 15, 2022
View reviewed changes
Comment thread sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/SdkMeter.java Outdated
jack-berg
jack-berg commented Apr 18, 2022
View reviewed changes
Comment thread api/all/src/main/java/io/opentelemetry/api/internal/ValidationUtil.java
@jack-berg jack-berg mentioned this pull request Apr 18, 2022
Merged
@jack-berg jack-berg closed this Apr 19, 2022
@jack-berg jack-berg reopened this Apr 19, 2022
anuraaga
anuraaga approved these changes Apr 20, 2022
View reviewed changes
Comment thread api/all/src/main/java/io/opentelemetry/api/metrics/DefaultMeter.java Outdated
@jack-berg jack-berg merged commit 98369a5 into open-telemetry:main Apr 20, 2022
@jack-berg jack-berg mentioned this pull request Jan 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkwatson jkwatson jkwatson left review comments

@jsuereth jsuereth Awaiting requested review from jsuereth

@Oberon00 Oberon00 Awaiting requested review from Oberon00

+2 more reviewers

@mateuszrzeszutek mateuszrzeszutek mateuszrzeszutek left review comments

@anuraaga anuraaga anuraaga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Enforce metric instrument naming rule

4 participants

@jack-berg @anuraaga @jkwatson @mateuszrzeszutek