-
Notifications
You must be signed in to change notification settings - Fork 814
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
feat(views): handle view conflicts. #2734
feat(views): handle view conflicts. #2734
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2734 +/- ##
==========================================
+ Coverage 93.41% 93.48% +0.06%
==========================================
Files 160 163 +3
Lines 5467 5541 +74
Branches 1149 1166 +17
==========================================
+ Hits 5107 5180 +73
- Misses 360 361 +1
|
Link here for visibility: open-telemetry/opentelemetry-specification#2226 |
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.
Great works 👍 ! I'd like to wait for open-telemetry/opentelemetry-specification#2270 to land since this would really depend on lots of statements in that spec change.
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/InstrumentDescriptor.ts
Outdated
Show resolved
Hide resolved
|
||
export function getDescriptorIncompatibilityDetails(existing: InstrumentDescriptor, otherDescriptor: InstrumentDescriptor) { | ||
let incompatibility = ''; | ||
if (existing.description !== otherDescriptor.description) { |
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.
Ditto.
@pichlermarc the spec team asked language sdks to prototype the changes proposed at open-telemetry/opentelemetry-specification#2317, would you mind making the changes? |
@legendecas yes, I will pick up work on this :) |
acdd6de
to
9300f52
Compare
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.
Nice work! 👍
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
Outdated
Show resolved
Hide resolved
79282df
to
cf680f6
Compare
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
Outdated
Show resolved
Hide resolved
experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricStorageRegistry.ts
Outdated
Show resolved
Hide resolved
@pichlermarc it seems like I cannot edit this PR (merge the base branch into this PR). Would you mind updating the PR with the latest base branch so that we could merge this one? |
Which problem is this PR solving?
This PR is part of the work on #2592 and adds a
MetricStorageRegistry
, as well as logic inMeter
to handle conflicts with view names that are used more than once perMeter
.Currently waiting for open-telemetry/opentelemetry-specification#2317 to be merged.
Type of change
How Has This Been Tested?
Checklist: