[sdk-metrics] ExemplarFilter updates to match latest specification #5404

CodeBlanch · 2024-03-01T05:44:10Z

Changes

Updates ExemplarFilter experimental API to match changes made to spec.

Merge requirement checklist

CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
Unit tests added/updated
Appropriate CHANGELOG.md files updated for non-trivial changes
Changes in public API reviewed (if applicable)

codecov · 2024-03-01T05:49:23Z

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 84.58%. Comparing base (6250307) to head (1c75977).
Report is 107 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5404      +/-   ##
==========================================
+ Coverage   83.38%   84.58%   +1.19%     
==========================================
  Files         297      284      -13     
  Lines       12531    12104     -427     
==========================================
- Hits        10449    10238     -211     
+ Misses       2082     1866     -216

Flag	Coverage Δ
unittests	`?`
unittests-Solution-Experimental	`84.50% <92.85%> (?)`
unittests-Solution-Stable	`84.55% <92.85%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...emetry/Metrics/Exemplar/AlwaysOffExemplarFilter.cs	`0.00% <ø> (ø)`
...lemetry/Metrics/Exemplar/AlwaysOnExemplarFilter.cs	`100.00% <ø> (ø)`
...metry/Metrics/Exemplar/TraceBasedExemplarFilter.cs	`50.00% <ø> (+50.00%)`	⬆️
.../Metrics/Builder/MeterProviderBuilderExtensions.cs	`98.47% <92.85%> (-1.53%)`	⬇️

... and 55 files with indirect coverage changes

cijothomas · 2024-03-01T16:44:26Z

src/OpenTelemetry/CHANGELOG.md

@@ -55,6 +55,16 @@
  API.
  ([#5396](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5396))

+* **Experimental (pre-release builds only):** Removed the `ExemplarFilter`,


Very nice wording! crisp and clear. 💯

src/OpenTelemetry/Metrics/Exemplar/ExemplarFilterType.cs

cijothomas · 2024-03-01T16:53:49Z

test/Benchmarks/Metrics/ExemplarBenchmarks.cs

    private MeterProvider meterProvider;
    private Meter meter;

    [System.Diagnostics.CodeAnalysis.SuppressMessage("StyleCop.CSharp.DocumentationRules", "SA1602:Enumeration items should be documented", Justification = "Test only.")]
-    public enum ExemplarFilterToUse
+    public enum ExemplarConfigurationType


can you update the benchmark numbers as well? This was added to highlight the relevance of filtering and how perf gets affected when more measurements are considered (not store) for exemplars?

I updated the numbers on here: 1c75977

But I will say, I think having the numbers in the source files is bad practice. The numbers are really only useful when run on the same machine and run against other numbers from the same machine run under similar circumstances. To properly benchmark, you need to run numbers on some branch, and then switch to a branch with code changes and re-run them again. Having the numbers in the source file could mislead people into thinking they have made things better or worse unless they correctly re-run the base first 🤔

These numbers are useful for tracking memory allocation. It's easy to check if your code adds or reduces the memory allocation by comparing with the results in the source file.

Having the numbers in the source file could mislead people into thinking they have made things better or worse unless they correctly re-run the base first

We also copy the information about the hardware config when running the benchmarks so if someone is ignoring that, then maybe we could just add a comment saying these numbers are related to that particular config only.

Probably not the way to do it. We don't even turn on memory allocation tracking by default you have to add "-m" 🤣 A solution would be more like we mark the things which we expect to be allocation-free and verify that in CI so we aren't reliant on a) people running them and b) people running them correctly understanding the nuances.

CI based solution would be much better than what we have for sure.

But with the existing approach, for anyone updating benchmarks in a PR, the reviewers check (and should check) if the memory allocation column is added or not. They also check if the PR author ran the benchmarks for both main and the feature branch.

we can still keep them with a note above.
Its very useful to look at it and see how much overhead each feature is adding on.

test/Benchmarks/Metrics/ExemplarBenchmarks.cs

src/OpenTelemetry/Metrics/Exemplar/AlwaysOffExemplarFilter.cs

cijothomas

Left some nits, nothing blocking and can be follow ups.

vishweshbankwar · 2024-03-01T17:08:13Z

@CodeBlanch This needs to be updated as well:

opentelemetry-dotnet/examples/AspNetCore/Program.cs

Line 88 in 42ecd73

.SetExemplarFilter(new TraceBasedExemplarFilter())

. Curious how did the build couldn't catch this?

vishweshbankwar · 2024-03-01T17:18:01Z

src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderExtensions.cs

+    /// Sets the <see cref="ExemplarFilterType"/> to be used for this provider
+    /// which controls how measurements will be offered to exemplar reservoirs.
+    /// Default provider configuration: <see
+    /// cref="ExemplarFilterType.AlwaysOff"/>.


The default is TraceBased

Edit: I see it says provider configuration. Could we reword this? If someone just does SetExemplarFilter(), the default would be TraceBased

need some spec work to clarify defaults. It is not clear if exemplars, as a feature, itself is enabled by default. once enabled, yes, tracebased is the default, but we need to see if that makes sense. (I don't think so, but will wait for Blanch's current work before we approach spec)

Agree it is a bit confusing in the spec.

There's two statements in metrics sdk:

Exemplar sampling SHOULD be turned off by default. If Exemplar sampling is off, the SDK MUST NOT have overhead related to exemplar sampling.

The ExemplarFilter SHOULD be a configuration parameter of a MeterProvider for an SDK. The default value SHOULD be TraceBased.

I tried to satisfy that 🤣

vishweshbankwar · 2024-03-01T17:33:58Z

src/OpenTelemetry/Metrics/Exemplar/ExemplarFilterType.cs

+
+    /// <summary>
+    /// An exemplar filter which makes measurements recorded in the context of a
+    /// sampled <see cref="Activity"/> (span) eligible for becoming an <see


Suggested change

/// sampled <see cref="Activity"/> (span) eligible for becoming an <see

/// sampled parent <see cref="Activity"/> (span) eligible for becoming an <see

to match the spec definition.

This "parent" text was in a couple spots. I removed it because I thought it was super confusing!

An ExemplarFilter which makes those measurements eligible for being an Exemplar, which are recorded in the context of a sampled parent span.

Does that mean Activity.Current.Parent.Recorded or Activity.Current.Recorded?

An ExemplarFilter which makes those measurements eligible for being an Exemplar, which are recorded in the context of a sampled span.

Seems to more clearly mean Activity.Current.Recorded?

I think the parent term is used with respect to the measurement here. But I see the confusion, ok to keep what you have.

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs

CodeBlanch · 2024-03-01T18:09:10Z

This needs to be updated as well:

opentelemetry-dotnet/examples/AspNetCore/Program.cs

Line 88 in 42ecd73

.SetExemplarFilter(new TraceBasedExemplarFilter())

. Curious how did the build couldn't catch this?

@vishweshbankwar

Updated here: ce1e34e

The build didn't catch it because #if EXPOSE_EXPERIMENTAL_FEATURES is meaningless to the example apps. Something I need to fix just haven't had a chance yet.

CodeBlanch added 2 commits February 29, 2024 21:36

ExemplarFilter updates to match specification.

2acba3a

Make ExemplarFilterType an experimental API.

18cfb2d

CodeBlanch added pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package metrics Metrics signal related labels Mar 1, 2024

CodeBlanch requested a review from a team March 1, 2024 05:44

CHANGELOG patch.

1073a12

cijothomas reviewed Mar 1, 2024

View reviewed changes

src/OpenTelemetry/Metrics/Exemplar/ExemplarFilterType.cs Outdated Show resolved Hide resolved

cijothomas reviewed Mar 1, 2024

View reviewed changes

src/OpenTelemetry/Metrics/Exemplar/ExemplarFilterType.cs Show resolved Hide resolved

cijothomas reviewed Mar 1, 2024

View reviewed changes

test/Benchmarks/Metrics/ExemplarBenchmarks.cs Show resolved Hide resolved

cijothomas reviewed Mar 1, 2024

View reviewed changes

src/OpenTelemetry/Metrics/Exemplar/AlwaysOffExemplarFilter.cs Show resolved Hide resolved

cijothomas approved these changes Mar 1, 2024

View reviewed changes

vishweshbankwar reviewed Mar 1, 2024

View reviewed changes

rajkumar-rangaraj reviewed Mar 1, 2024

View reviewed changes

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpMetricsExporterTests.cs Show resolved Hide resolved

Code review.

0884011

rajkumar-rangaraj approved these changes Mar 1, 2024

View reviewed changes

CodeBlanch added 2 commits March 1, 2024 10:04

Code review.

ce1e34e

Doc updates.

af79723

Test case.

6963b0c

vishweshbankwar approved these changes Mar 1, 2024

View reviewed changes

Benchmark improvements.

1c75977

cijothomas approved these changes Mar 1, 2024

View reviewed changes

CodeBlanch merged commit b754b13 into open-telemetry:main Mar 1, 2024
37 checks passed

CodeBlanch deleted the sdk-metrics-exemplarfilter-spec-updates branch March 1, 2024 19:42

CodeBlanch mentioned this pull request Mar 4, 2024

[sdk-metrics] Remove ExemplarFilter classes #5408

Merged

2 tasks

CodeBlanch mentioned this pull request Mar 13, 2024

Exemplar - remove ExemplarFilter API #5333

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk-metrics] ExemplarFilter updates to match latest specification #5404

[sdk-metrics] ExemplarFilter updates to match latest specification #5404

CodeBlanch commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

cijothomas Mar 1, 2024

cijothomas Mar 1, 2024

CodeBlanch Mar 1, 2024

utpilla Mar 1, 2024

CodeBlanch Mar 1, 2024

utpilla Mar 1, 2024 •

edited

Loading

cijothomas Mar 1, 2024

cijothomas left a comment

vishweshbankwar commented Mar 1, 2024

vishweshbankwar Mar 1, 2024 •

edited

Loading

cijothomas Mar 1, 2024

CodeBlanch Mar 1, 2024

vishweshbankwar Mar 1, 2024

CodeBlanch Mar 1, 2024

vishweshbankwar Mar 1, 2024

CodeBlanch commented Mar 1, 2024

	/// sampled <see cref="Activity"/> (span) eligible for becoming an <see
	/// sampled parent <see cref="Activity"/> (span) eligible for becoming an <see

[sdk-metrics] ExemplarFilter updates to match latest specification #5404

[sdk-metrics] ExemplarFilter updates to match latest specification #5404

Conversation

CodeBlanch commented Mar 1, 2024 • edited Loading

Changes

Merge requirement checklist

codecov bot commented Mar 1, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cijothomas left a comment

Choose a reason for hiding this comment

vishweshbankwar commented Mar 1, 2024

vishweshbankwar Mar 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Mar 1, 2024

CodeBlanch commented Mar 1, 2024 •

edited

Loading

codecov bot commented Mar 1, 2024 •

edited

Loading

utpilla Mar 1, 2024 •

edited

Loading

vishweshbankwar Mar 1, 2024 •

edited

Loading