Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Global error handler cleanup - Metrics SDK #2185
Global error handler cleanup - Metrics SDK #2185
Changes from 1 commit
704b848
b8cb6af
a0b6eee
acf97fa
7b48f14
ac61b79
a42d516
dbaa7f5
73fca4d
ee5c5f5
3aa97cf
de54afe
e62de04
bda9faa
4a34922
0087c24
115e73f
56682a4
7e48cbf
d81b374
4b09c92
949930e
6dcc9eb
4b42fe8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 152 in opentelemetry-sdk/src/metrics/internal/mod.rs
opentelemetry-sdk/src/metrics/internal/mod.rs#L151-L152
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree Error is the right severity for this, but this is still not so user-friendly.
Since this is user facing, the internal log should be clearly actionable, along with the impact of this error.
In this case, the impact is - measurements reported using this instrument will be ignore.
The "why" is also not so obvious.
As an end-user, I'd prefer to see something like
Name: InstrumentCreationFailed
InstrumentName: A_non_asccii_Metric
MeterName: MeterName
Reason: Instrument Name contains non-ascii charactors. Link to spec, optionally.
Message: Measurements from this instruments will be ignored.
Check warning on line 93 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L93
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as https://github.com/open-telemetry/opentelemetry-rust/pull/2185/files#r1796240076
I dont think a user would know the difference between ValidationError vs Error.
Check warning on line 122 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L122
Check warning on line 162 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L162
Check warning on line 202 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L202
Check warning on line 243 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L243
Check warning on line 275 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L275
Check warning on line 307 in opentelemetry-sdk/src/metrics/meter.rs
opentelemetry-sdk/src/metrics/meter.rs#L307
Check warning on line 139 in opentelemetry-sdk/src/metrics/meter_provider.rs
opentelemetry-sdk/src/metrics/meter_provider.rs#L139
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can skip logging this set of attributes twice. The event name says that we have a duplicate stream so it should already be understood that we have two identical sets of attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not very sure of the logic here. Can you check once. As we return at line 414 if they are same.
Check warning on line 426 in opentelemetry-sdk/src/metrics/pipeline.rs
opentelemetry-sdk/src/metrics/pipeline.rs#L416-L426
Check warning on line 108 in opentelemetry-sdk/src/metrics/view.rs
opentelemetry-sdk/src/metrics/view.rs#L105-L108
Check warning on line 120 in opentelemetry-sdk/src/metrics/view.rs
opentelemetry-sdk/src/metrics/view.rs#L117-L120
Check warning on line 148 in opentelemetry-sdk/src/metrics/view.rs
opentelemetry-sdk/src/metrics/view.rs#L146-L148