Skip to content

[sdk] Hardcode known histograms using seconds #4820

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.

Sign up for GitHub

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

Merged
utpilla merged 8 commits into from
Sep 5, 2023
Merged

[sdk] Hardcode known histograms using seconds #4820

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d56ea68
Add default bucket boundary
samlam Sep 1, 2023
685f352
Added OTEL meter/instrument names
samlam Sep 1, 2023
e5ebe25
Add histogram buckets default for seconds unit test
samlam Sep 1, 2023
813d4ff
minor clean up
samlam Sep 1, 2023
73f3995
further clean up + adding non-default test case
samlam Sep 5, 2023
bd85286
Update src/OpenTelemetry/Metrics/Metric.cs
samlam Sep 5, 2023
91bcbcf
Update src/OpenTelemetry/Metrics/Metric.cs
samlam Sep 5, 2023
7d38c10
Merge branch 'main' into features/default-buckets
utpilla Sep 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion src/OpenTelemetry/Metrics/AggregatorStore.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ internal AggregatorStore(
this.currentMetricPointBatch = new int[maxMetricPoints];
this.aggType = aggType;
this.outputDelta = temporality == AggregationTemporality.Delta;
this.histogramBounds = metricStreamIdentity.HistogramBucketBounds ?? Metric.DefaultHistogramBounds;
this.histogramBounds = metricStreamIdentity.HistogramBucketBounds ?? FindDefaultHistogramBounds(in metricStreamIdentity);
this.exponentialHistogramMaxSize = metricStreamIdentity.ExponentialHistogramMaxSize;
this.exponentialHistogramMaxScale = metricStreamIdentity.ExponentialHistogramMaxScale;
this.StartTimeExclusive = DateTimeOffset.UtcNow;
Expand Down Expand Up @@ -106,6 +106,8 @@ internal AggregatorStore(

internal DateTimeOffset EndTimeInclusive { get; private set; }

internal double[] HistogramBounds => this.histogramBounds;

internal bool IsExemplarEnabled()
{
// Using this filter to indicate On/Off
Expand Down Expand Up @@ -196,6 +198,17 @@ internal void SnapshotCumulative(int indexSnapshot)
internal MetricPointsAccessor GetMetricPoints()
=> new(this.metricPoints, this.currentMetricPointBatch, this.batchSize);

private static double[] FindDefaultHistogramBounds(in MetricStreamIdentity metricStreamIdentity)
{
if (metricStreamIdentity.Unit == "s" && Metric.DefaultHistogramBoundMappings
.Contains((metricStreamIdentity.MeterName, metricStreamIdentity.InstrumentName)))
{
return Metric.DefaultHistogramBoundsSeconds;
}

return Metric.DefaultHistogramBounds;
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void InitializeZeroTagPointIfNotInitialized()
{
Expand Down
16 changes: 16 additions & 0 deletions src/OpenTelemetry/Metrics/Metric.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Provide the capacity in the ctor

{
("Microsoft.AspNetCore.Hosting", "http.server.request.duration"),
("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration"),
("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request.time_in_queue"),
("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"),
Copy link
Copy Markdown
Member

@vishweshbankwar vishweshbankwar Sep 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Comment thread
samlam marked this conversation as resolved.
Outdated
("OpenTelemetry.Instrumentation.Http", "http.client.duration"),
Comment thread
samlam marked this conversation as resolved.
Outdated
("System.Net.Http", "http.client.connection.duration"),
Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

("System.Net.Http", "http.client.request.duration"),
("System.Net.Http", "http.client.request.time_in_queue"),
("System.Net.NameResolution", "dns.lookups.duration"),
};

private readonly AggregatorStore aggStore;

Expand Down
44 changes: 44 additions & 0 deletions test/OpenTelemetry.Tests/Metrics/AggregatorTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,50 @@ public void MultiThreadedHistogramUpdateAndSnapShotTest()
Assert.Equal(200, argsToThread.SumOfDelta + lastDelta);
}

[Theory]
[InlineData("Microsoft.AspNetCore.Hosting", "http.server.request.duration")]
[InlineData("Microsoft.AspNetCore.Http.Connections", "signalr.server.connection.duration")]
[InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request_lease.duration")]
Comment thread
samlam marked this conversation as resolved.
[InlineData("Microsoft.AspNetCore.RateLimiting", "aspnetcore.rate_limiting.request.time_in_queue")]
[InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.connection.duration")]
[InlineData("Microsoft.AspNetCore.Server.Kestrel", "kestrel.tls_handshake.duration")]
[InlineData("OpenTelemetry.Instrumentation.AspNetCore", "http.server.duration")]
[InlineData("OpenTelemetry.Instrumentation.Http", "http.client.duration")]
[InlineData("System.Net.Http", "http.client.connection.duration")]
[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")]
[InlineData("General.App", "simple.alternative.counter")]
public void HistogramBucketsDefaultUpdatesForSecondsTest(string meterName, string instrumentName)
Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
RunTest(meterName, instrumentName, unit: "s");
RunTest(meterName, instrumentName, unit: "ms");
RunTest(meterName, instrumentName, unit: "By");
RunTest(meterName, instrumentName, unit: null);

void RunTest(string meterName, string instrumentName, string unit)
{
using var meter = new Meter(meterName);

var instrument = meter.CreateHistogram<double>(instrumentName, unit);

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

AggregatorStore aggregatorStore = new(
Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@samlam samlam Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@cijothomas cijothomas Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@cijothomas cijothomas Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Member

@CodeBlanch CodeBlanch Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Copy Markdown
Contributor

@utpilla utpilla Sep 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

metricStreamIdentity,
AggregationType.Histogram,
AggregationTemporality.Cumulative,
maxMetricPoints: 1024,
this.emitOverflowAttribute);

Assert.NotNull(aggregatorStore.HistogramBounds);
Assert.Equal(
unit == "s" && Metric.DefaultHistogramBoundMappings.Contains((meterName, instrumentName)) ?
Metric.DefaultHistogramBoundsSeconds : Metric.DefaultHistogramBounds,
aggregatorStore.HistogramBounds);
}
}

internal static void AssertExponentialBucketsAreCorrect(Base2ExponentialBucketHistogram expectedHistogram, ExponentialHistogramData data)
{
Assert.Equal(expectedHistogram.Scale, data.Scale);
Expand Down