-
Notifications
You must be signed in to change notification settings - Fork 773
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
[sdk-metrics] Turn exemplars on by default in prerelease builds #5545
[sdk-metrics] Turn exemplars on by default in prerelease builds #5545
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5545 +/- ##
==========================================
+ Coverage 83.38% 85.61% +2.23%
==========================================
Files 297 289 -8
Lines 12531 12493 -38
==========================================
+ Hits 10449 10696 +247
+ Misses 2082 1797 -285
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM - left a suggestion for changelog.
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 okay with this change, but I want to see if we can make a NoopExemplarReservoir, and make it as the default for non-histograms (spec is flexible to allow that) in the 1st stable release.
src/OpenTelemetry/CHANGELOG.md
Outdated
@@ -12,6 +12,11 @@ | |||
function when configuring a view (applies to individual metrics). | |||
([#5542](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5542)) | |||
|
|||
* **Experimental (pre-release builds only):** The default `ExemplarFilterType` | |||
on `MeterProvider` is now `ExemplarFilterType.TraceBased` which will enable |
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.
What's the perf implications that the users should be aware of?
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.
Let's wait on this. My guess is most users will skip over anything prefixed with **Experimental (pre-release builds only):**
in the CHANGELOG. What I think would be more useful is on the final entry where we make everything public for stable builds we can add a link there to something in the docs. Thinking like: Understanding performance implications when sampling Exemplars
.
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 fine to address the changelog/doc later.
I think we still need to know the perf implication of this PR as it serves as a critical input while making decisions.
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.
Updated the description with some benchmarks.
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.
Could you also provide the increase in memory consumption per metric since we would now allocate an ExemplarReservoir
instance for each MetricPoint? It could be an issue for histogram users with high bucket counts.
After looking at the perf numbers, the overhead is non-trivial. So I recommend to keep it off by default for every metric. Users can opt-in to each metric (using views). (Not many backends/venodors are known to support exemplars.) |
Going to keep exemplars off by default for now based on performance analysis. |
Changes
ExemplarFilterType
toTraceBased
in prerelease builds to match spec.Benchmarks
Counters (SimpleFixedSizeExemplarReservoir)
Using
TraceBased
WITHOUT an active trace has some cost (we need to checkActivity.Current.Recorded
in the hot path):Using
TraceBased
WITH an active trace has more cost (we need to checkActivity.Current.Recorded
and do a random-based sample in the hot path):Histograms (AlignedHistogramBucketExemplarReservoir)
Using
TraceBased
WITHOUT an active trace is interesting. Sometimes I run it things show faster, sometimes it shows slower, and sometimes it shows mixed. I take this as statistically no difference. The cost of the check forActivity.Current.Recorded
is dwarfed by the other work to find the bucket and do all the updating:Using
TraceBased
WITH an active trace has a lot of cost (we need to checkActivity.Current.Recorded
and we always update exemplar for every measurement in the hot path):This is an interesting area @cijothomas and I have discussed. The spec says for
AlignedHistogramBucketExemplarReservoir
always keep the last exemplar seen for a bucket. There's a lot of overriding as a result (wasted cycles). A simple thing to do would be keep only the first exemplar for a given export. Or do something more likeSimpleFixedSizeExemplarReservoir
where we always keep the first one then randomly decide whether or not to keep subsequent exemplars 🤔Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes