Skip to content
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

Updating ref docs to include the metrics bucket information for connection histograms #37811

Merged
merged 4 commits into from
Feb 1, 2024

Conversation

samsp-msft
Copy link
Member

@samsp-msft samsp-msft commented Oct 31, 2023

Summary

We default histogram buckets to a size that makes sense for requests, but is not ideal for connections. This change provides longer durations for connections so that the upper bound is 5min. Connections may last longer than that, but the performance difference will be negligible after 10s of seconds, so seems like a good upper bound, while still providing granularity at the lower values, and using an approximate doubling of each bucket size.

Related to open-telemetry/opentelemetry-dotnet#5008,
open-telemetry/opentelemetry-dotnet#4922


Internal previews

📄 File 🔗 Preview link
docs/core/diagnostics/built-in-metrics-aspnetcore.md ASP.NET Core metrics
docs/core/diagnostics/built-in-metrics-system-net.md System.Net metrics

@samsp-msft samsp-msft requested review from tommcdon and a team as code owners October 31, 2023 04:16
@dotnet-bot dotnet-bot added this to the October 2023 milestone Oct 31, 2023
@samsp-msft samsp-msft changed the title Updating to include the metrics bucket information for Updating to include the metrics bucket information for connection histograms Oct 31, 2023
@samsp-msft samsp-msft changed the title Updating to include the metrics bucket information for connection histograms Updating ref docs to include the metrics bucket information for connection histograms Oct 31, 2023
@noahfalk
Copy link
Member

Adding link to JamesNK's similar change to the OTel docs: open-telemetry/semantic-conventions#283 (review)

@JamesNK
Copy link
Member

JamesNK commented Nov 1, 2023

I think this level of detail is unnecessary in the .NET docs. Most people won't know what histogram buckets are. The people who do care can look at the spec in the OTEL repo for more information.

Updated to square brackets to be in sync with the OTel docs.
@IEvangelist IEvangelist enabled auto-merge (squash) February 1, 2024 20:10
@IEvangelist IEvangelist merged commit 58ca801 into main Feb 1, 2024
8 of 9 checks passed
@IEvangelist IEvangelist deleted the samsp/metrics-buckets branch February 1, 2024 20:18
@JamesNK
Copy link
Member

JamesNK commented Feb 1, 2024

I said this wasn’t necessary?

@gewarren
Copy link
Contributor

gewarren commented Feb 2, 2024

@IEvangelist Revert?

@IEvangelist
Copy link
Member

@gewarren - I guess, I also don't think it hurts to have it. It seems valuable, I'd personally rather read this alongside the .NET bits that discuss this sort of thing, and I would likely avoid reading the OTEL spec. That's why I merged it... Y'all can revert it if you think it's really that unnecessary, otherwise leave it. 🤷🏼‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants