[sdk] Hardcode known histograms using seconds #4820

samlam · 2023-09-01T21:53:41Z

Closes #4797
Design discussion issue #4797

Changes

Change histogram default boundaries when MetricStreamIdentity.Unit is set to "s"
New default buckets are set to { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 }
Created a set of well-known Meters/Instruments for the default
Added unit-test for the new defaults

Acknowledgement

The PR is created with the assistance from @CodeBlanch

Merge requirement checklist

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

linux-foundation-easycla · 2023-09-01T21:53:44Z

The committers listed above are authorized under a signed CLA.

✅ login: samlam / name: Samuel Lam (d56ea68, 685f352, e5ebe25, 813d4ff, 73f3995, bd85286, 91bcbcf)
✅ login: utpilla / name: Utkarsh Umesan Pillai (7d38c10)

src/OpenTelemetry/Metrics/AggregatorStore.cs

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs

src/OpenTelemetry/Metrics/AggregatorStore.cs

codecov · 2023-09-03T05:35:02Z

Codecov Report

Merging #4820 (91bcbcf) into main (34544bd) will increase coverage by 0.05%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 91bcbcf differs from pull request most recent head 7d38c10. Consider uploading reports for the commit 7d38c10 to get more accurate results

@@            Coverage Diff             @@
##             main    #4820      +/-   ##
==========================================
+ Coverage   83.86%   83.92%   +0.05%     
==========================================
  Files         293      293              
  Lines       11977    11998      +21     
==========================================
+ Hits        10045    10069      +24     
+ Misses       1932     1929       -3

Files Changed	Coverage
src/OpenTelemetry/Metrics/AggregatorStore.cs	`100.00%`
src/OpenTelemetry/Metrics/Metric.cs	`100.00%`

src/OpenTelemetry/Metrics/AggregatorStore.cs

utpilla · 2023-09-03T05:39:49Z

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs

+ [InlineData("System.Net.Http", "http.client.request.duration")]
+ [InlineData("System.Net.Http", "http.client.request.time_in_queue")]
+ [InlineData("System.Net.NameResolution", "dns.lookups.duration")]
+ public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, string instrumentName)


Add another unit test to check that a regular Histogram with Unit as seconds still uses the default Histogram bounds.

Is this resolved? I don't see another unit test added.

utpilla · 2023-09-03T05:44:04Z

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs

+
+ var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);
+
+ AggregatorStore aggregatorStore = new(


This way of testing is relying a bit too much on the knowledge of the internal implementation. Could you use test the bounds using an InMemoryExporter instead? Get hold of the MetricPoint after the export and check the bounds using metricPoint.GetHistogramBuckets().ExplicitBounds.

I'm not familiar with InMemoryExporter, let me research it a bit. Thx

This way of testing is relying a bit too much on the knowledge of the internal implementation.

This logic is strange to me. It's a unit test. We're changing how AggregatorStore resolves buckets. That's the "unit" we are testing. Logic downstream reliant on the resolved buckets isn't being changed. Other tests should be covering that. I would very much expect unit tests to cover internal details 😄

If we want a bigger integration test exercising more of the SDK, fine to do, but too much of that is going to result in slow CI with a ton of overlap. Just my $0.02 😄

That's the "unit" we are testing.

We haven't really followed the pedantic approach to unit testing in our repo. It's a spectrum. The tests in our repo are always somewhere between a strict by-the-book unit test and a blanket integration test.

Please make sure to add tests to cover that this special logic is only applicable if SDK is using defaults. i.e if a View is configured to override buckets, then that gets respected in all metrics, including the ones we are hardcoding.

Please make sure to add tests to cover that this special logic is only applicable if SDK is using defaults. i.e if a View is configured to override buckets, then that gets respected in all metrics, including the ones we are hardcoding.

And I recommend to the existing pattern of using InMemoryExporter to achieve this, though I fully agree its neither unit nor integration tests but something in between!

Please make sure to add tests to cover that this special logic is only applicable if SDK is using defaults. i.e if a View is configured to override buckets, then that gets respected in all metrics, including the ones we are hardcoding.

Why is that needed? This code is only touching the default bounds which are used if there wasn't a view. We have view tests covering view bounds are selected over the defaults (whatever they may be).

@samlam You could try extending this test. But not sure how to best solve needing an exact meter name.

We haven't really followed the pedantic approach to unit testing in our repo. It's a spectrum. The tests in our repo are always somewhere between a strict by-the-book unit test and a blanket integration test.

Well we are the maintainers. If we ask people to add integration tests that's what we'll get 😄 And we'll have to maintain the extra code forever!

This code is only touching the default bounds which are used if there wasn't a view.

We know this to be true but it would be good to test that and verify that Views take precedence even for especially treated metrics.

vishweshbankwar · 2023-09-05T17:25:37Z

src/OpenTelemetry/Metrics/Metric.cs

+ ("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request_lease.duration"),
+ ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration"),
+ ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration"),
+ ("OpenTelemetry.Instrumentation.AspNetCore", "http.server.duration"),


http.server.duration and http.client.duration should not be part of this list. Those will continue to follow default buckets with unit as ms. #4802.

This should be
("OpenTelemetry.Instrumentation.AspNetCore", "http.server.request.duration")
("OpenTelemetry.Instrumentation.HttpClient", "http.client.request.duration")

src/OpenTelemetry/Metrics/Metric.cs

utpilla · 2023-09-05T19:27:55Z

src/OpenTelemetry/Metrics/Metric.cs

@@ -28,6 +28,22 @@ public sealed class Metric
 internal const int DefaultExponentialHistogramMaxScale = 20;

 internal static readonly double[] DefaultHistogramBounds = new double[] { 0, 5, 10, 25, 50, 75, 100, 250, 500, 750, 1000, 2500, 5000, 7500, 10000 };
+ internal static readonly double[] DefaultHistogramBoundsSeconds = new double[] { 0, 0.005, 0.01, 0.025, 0.05, 0.075, 0.1, 0.25, 0.5, 0.75, 1, 2.5, 5, 7.5, 10 };
+ internal static readonly HashSet<(string, string)> DefaultHistogramBoundMappings = new()


nit: Provide the capacity in the ctor

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>

utpilla

Approving this as the changes made in the src/ look good to me and we would like to unblock 1.6.0 release.

The unresolved comments should be addressed in later PRs.

CodeBlanch

LGTM thanks @samlam

utpilla · 2023-09-05T22:16:42Z

src/OpenTelemetry/Metrics/Metric.cs

+ ("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration"),
+ ("OpenTelemetry.Instrumentation.AspNetCore", "http.server.request.duration"),
+ ("OpenTelemetry.Instrumentation.Http", "http.client.request.duration"),
+ ("System.Net.Http", "http.client.connection.duration"),


It would be great to update the PR description with the links to the runtime code for where these metrics are defined.

samlam added 3 commits September 1, 2023 00:27

Add default bucket boundary

d56ea68

Added OTEL meter/instrument names

685f352

Add histogram buckets default for seconds unit test

e5ebe25

samlam changed the title ~~Features/default buckets~~ [sdk] Hardcode known histograms using seconds Sep 1, 2023

samlam marked this pull request as ready for review September 1, 2023 22:01

samlam requested a review from a team September 1, 2023 22:01

reyang reviewed Sep 1, 2023

View reviewed changes

src/OpenTelemetry/Metrics/AggregatorStore.cs Outdated Show resolved Hide resolved

reyang reviewed Sep 1, 2023

View reviewed changes

test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs Show resolved Hide resolved

minor clean up

813d4ff

utpilla reviewed Sep 3, 2023

View reviewed changes

src/OpenTelemetry/Metrics/AggregatorStore.cs Outdated Show resolved Hide resolved

utpilla reviewed Sep 3, 2023

View reviewed changes

src/OpenTelemetry/Metrics/AggregatorStore.cs Outdated Show resolved Hide resolved

utpilla reviewed Sep 3, 2023

View reviewed changes

further clean up + adding non-default test case

73f3995

vishweshbankwar reviewed Sep 5, 2023

View reviewed changes

utpilla reviewed Sep 5, 2023

View reviewed changes

src/OpenTelemetry/Metrics/Metric.cs Outdated Show resolved Hide resolved

utpilla reviewed Sep 5, 2023

View reviewed changes

src/OpenTelemetry/Metrics/Metric.cs Outdated Show resolved Hide resolved

utpilla reviewed Sep 5, 2023

View reviewed changes

samlam and others added 3 commits September 5, 2023 12:33

Update src/OpenTelemetry/Metrics/Metric.cs

bd85286

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>

Update src/OpenTelemetry/Metrics/Metric.cs

91bcbcf

Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>

Merge branch 'main' into features/default-buckets

7d38c10

utpilla approved these changes Sep 5, 2023

View reviewed changes

CodeBlanch approved these changes Sep 5, 2023

View reviewed changes

utpilla reviewed Sep 5, 2023

View reviewed changes

utpilla merged commit a0084f2 into open-telemetry:main Sep 5, 2023
27 checks passed

utpilla mentioned this pull request Sep 5, 2023

Update CHANGELOG for 1.6.0 #4827

Merged

cijothomas mentioned this pull request Sep 14, 2023

HttpSemanticConventions - new metric in AspNetCore Instrumentation. #4802

Merged

4 tasks

vishweshbankwar mentioned this pull request Oct 2, 2023

Default values for explicit histogram bucket boundaries for fixed set of metrics #4908

Closed

jakegavin mentioned this pull request Sep 13, 2024

Default values for histogram bucket boundaries are oriented around milliseconds rather than seconds open-telemetry/opentelemetry-go#5821

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sdk] Hardcode known histograms using seconds #4820

[sdk] Hardcode known histograms using seconds #4820

samlam commented Sep 1, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 3, 2023 •

edited

Loading

utpilla Sep 3, 2023

utpilla Sep 5, 2023

utpilla Sep 3, 2023

samlam Sep 5, 2023

CodeBlanch Sep 5, 2023

utpilla Sep 5, 2023

cijothomas Sep 5, 2023

cijothomas Sep 5, 2023

CodeBlanch Sep 5, 2023

utpilla Sep 5, 2023

vishweshbankwar Sep 5, 2023 •

edited

Loading

utpilla Sep 5, 2023

utpilla left a comment

CodeBlanch left a comment

utpilla Sep 5, 2023


		var metricStreamIdentity = new MetricStreamIdentity(instrument, metricStreamConfiguration: null);

		AggregatorStore aggregatorStore = new(

[sdk] Hardcode known histograms using seconds #4820

[sdk] Hardcode known histograms using seconds #4820

Conversation

samlam commented Sep 1, 2023 • edited Loading

Changes

Acknowledgement

Merge requirement checklist

linux-foundation-easycla bot commented Sep 1, 2023 • edited Loading

codecov bot commented Sep 3, 2023 • 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

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

vishweshbankwar Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utpilla left a comment

Choose a reason for hiding this comment

CodeBlanch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samlam commented Sep 1, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 3, 2023 •

edited

Loading

vishweshbankwar Sep 5, 2023 •

edited

Loading