-
Notifications
You must be signed in to change notification settings - Fork 774
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
Remove meters from storage list when removed and handle NoRecordedValue flag when removing meters #5997
base: main
Are you sure you want to change the base?
Remove meters from storage list when removed and handle NoRecordedValue flag when removing meters #5997
Conversation
Hi @stonkie, thanks for the contribution. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5997 +/- ##
==========================================
- Coverage 84.14% 83.95% -0.20%
==========================================
Files 275 275
Lines 12560 12603 +43
==========================================
+ Hits 10569 10581 +12
- Misses 1991 2022 +31
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@Kielek : I'm going through the motions to get this done at my company and there are more hoops than I expected. EasyCLA will probably wait until Monday. |
Discussions during SIG meeting led to asking to feature flag the NoRecordedValue DataPoint to avoid changing working behavior. I'll add that and adjust the tests asap. |
…RDED_VALUE to only emil no recorded value data points as a opt-in feature
…om/stonkie/opentelemetry-dotnet into user/kcoulombe/HandleMeterDisposal
I'm not exactly sure how to format the changelog entry. Should I create an entry without version number or release date? |
Please put it under unreleased section. ## Unreleased
* Your description goes here.
And here if it is long enough.
([#5997](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5997))
``` |
…om/stonkie/opentelemetry-dotnet into user/kcoulombe/HandleMeterDisposal
@Kielek Thanks, it's done. |
} | ||
|
||
var exponentialHistogramData = metricPoint.GetExponentialHistogramData(); | ||
writePosition = WriteExponentialHistogramDataPoint(buffer, writePosition, in metricPoint); |
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.
There is a lot of refactoring in this file that is unrelated to the proposed changes. Could you please refactor these outside of this current PR? Doing so either before or after this PR merges would be fine.
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'm off for the holidays. I'll look into that first week of January.
**Limitation:** This means that quickly recreating meters within the same | ||
collection cycle will still exhaust the storage limit. | ||
|
||
* Added an experimental flag |
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.
This should move to OTLP Exporter changelog.
To make the review process easier, could you please separate the SDK and OTLP Exporter changes into different PRs? I recommend finishing off the SDK part first and then moving to the OTLP Exporter changes. Additionally, it would be helpful to keep the refactoring outside the scope of these PRs. |
Fixes #5950
Design discussion issue #
Changes
This allows clean Dispose of Meter objects which will send NoRecordedValue data points to close the series (e.g. send a staleness marker to prometheus). It also fixes the internal storage leak which would prevent frequent rotation of the metrics.
NOTE : A significant refactoring of the protobuf serialization merged right after I created this PR, so I had to redo a significant part of the change on Dec 4th.
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes