Add ExemplarFilter interfaces #3852
Conversation
| /// </summary> | ||
| internal sealed class NoneExemplarFilter : IExemplarFilter | ||
| { | ||
| public bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object>> tags) |
There was a problem hiding this comment.
I suggest having two performance test cases to guide the initial exploration:
- A
NoneExemplarFilter(orAlwaysOffExemplarFilter) is being used, and the performance should be the same as if such filter doesn't exist (in other words, folks don't have to pay any extra cost if they don't use exemplar at all). - A naive exemplar which only picks the 1st item, and try to compare the performance between a hard-wired approach (by just manually putting the logic directly without leveraging any abstraction/interface) and the pluggable approach (e.g. via interface/DI/etc.) - and if the pluggable approach is introducing too much extra overhead, rethink about the design (e.g. maybe switch to a different pluggable model).
There was a problem hiding this comment.
yup thats roughly what I plan. 1st goal is to get the shape of things, then attach it to right places and measure the perf..
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3852 +/- ##
==========================================
- Coverage 85.62% 84.66% -0.97%
==========================================
Files 289 298 +9
Lines 11255 11436 +181
==========================================
+ Hits 9637 9682 +45
- Misses 1618 1754 +136
|
| /// <c>false</c> to indicate this measurement is not eligible to become Exemplar | ||
| /// and will not be given to the ExemplarReservoir. | ||
| /// </returns> | ||
| bool ShouldSample(long value, ReadOnlySpan<KeyValuePair<string, object>> tags); |
There was a problem hiding this comment.
Do we need an explicit parameter for timestamp? When would this method be called with respect to when a metric is recorded?
There was a problem hiding this comment.
spec says timestamp is needed. but we do not allow user to provide timestamp for metric APIs, and is always assumed to be the current time.. so the Filter can also take current timestamp, without we providing it. (have to check if this is bad for perf.. )
(Similar with context. we dont pass it. its implicit.
There was a problem hiding this comment.
No concerns with thread preemption here, right?
…thomas/opentelemetry-dotnet into cijothomas/exemplar-filter1
| /// <summary> | ||
| /// The interface defining Exemplar Filter. | ||
| /// </summary> | ||
| internal interface IExemplarFilter |
There was a problem hiding this comment.
Is "filter" coming from the spec? Asking because IExemplarSampler, AlwaysOnExemplarSampler, & AlwaysOffExemplarSampler might fit in better with what we have for tracing 🤷
There was a problem hiding this comment.
open-telemetry/opentelemetry-specification#2919 I am proposing fixes in spec in parallel.
|
Marking as draft. No need to merge now. I will update once the perf implications are fully understood with basic implementations. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
github-actions
bot
removed
the
Stale
|
I was planning to merge changes to main after 1.4.0 ship has sailed. Unfortunately 1.4.0 has not yet happened. I'll be creating a new branch for metrics, and maintain that temporarily until 1.4.0 is out. Starting next week, will be sending PRs to merge to the metrics branch. This is done to unblock Exemplar feature work. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Towards #2527
Adds ExemplarFilter interface/default implnm.
Everything is internal, and is not connected to the Metrics SDK. This will be done post 1.4.0 stable release.
Ref: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplarfilter