Remove meters from storage list when removed and handle NoRecordedValue flag when removing meters #5997
Conversation
github-actions
bot
added
pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol
|
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. |
stonkie
changed the title
| } | ||
|
|
||
| var exponentialHistogramData = metricPoint.GetExponentialHistogramData(); | ||
| writePosition = WriteExponentialHistogramDataPoint(buffer, writePosition, in metricPoint); |
There was a problem hiding this comment.
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.
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.
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.mdfiles updated for non-trivial changes