[sdk] Customize known connection histograms buckets #5008
Conversation
|
FYI @samsp-msft @noahfalk In .NET 9 and later, I expect this customization will happen against the instrument in the .NET source code, and OTEL will automatically pick it up. Sam, I went with your initial numbers. While a connection could be longer than 5 minutes, it's a reasonable upper bound. They could be tweaked based on feedback. |
JamesNK
changed the title
JamesNK
changed the title
|
Is it possible for this change to make it into the 1.7.0 release? |
@JamesNK We could push for this change to be included in 1.7.0 release but could this requirement for a selective histogram bucket selection be documented? Preferably in open-telemetry/semantic-conventions#283 or at least in the .NET docs: ASP .NET Core and System.Net Metrics. We could then comfortably refer to that to make a case for this change. |
Codecov Report
@@ Coverage Diff @@
## main #5008 +/- ##
=======================================
Coverage 83.42% 83.42%
=======================================
Files 296 296
Lines 12377 12385 +8
=======================================
+ Hits 10325 10332 +7
- Misses 2052 2053 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
| ("System.Net.Http", "http.client.request.duration"), | ||
| ("System.Net.Http", "http.client.request.time_in_queue"), | ||
| ("System.Net.NameResolution", "dns.lookups.duration"), | ||
| }; | ||
|
|
||
| // Long default histogram bounds. Not based on a standard. May change in the future. | ||
| internal static readonly double[] DefaultHistogramBoundsLongSeconds = new double[] { 0, 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300 }; |
There was a problem hiding this comment.
Let's remove the bucket with the value 0?
There was a problem hiding this comment.
The other buckets bounds have a zero value. I think this PR should focus on one thing, and if removing zero makes sense then it can be a new PR.
There was a problem hiding this comment.
FYI - we will fix the other buckets as well for 1.7.0 release
#4908
There was a problem hiding this comment.
I don't mind if its one PR or two, but we should fix now while we can, and before it gets adopted - although I don't think its a breaking change.
Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com>
Fixes #4922
Design discussion issue #4922
Changes
0, 0.01, 0.02, 0.05, 0.1, 0.2, 0.5, 1, 2, 5, 10, 30, 60, 120, 300. 10 seconds upper bound is good for HTTP requests but too short for useful information about most connections. 300 seconds (5 minutes) upper bound is better.Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes