-
Notifications
You must be signed in to change notification settings - Fork 774
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
Metrics customization guide with .NET 9 Advice API details #5911
Conversation
@CodeBlanch Please review the changes and provide necessary feedback. |
With the introduction of the .NET 9 Advice API, developers can | ||
customize histogram metrics more effectively. The Advice API | ||
allows for explicit bucket boundaries when defining histograms, | ||
leading to more precise and informative metrics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Customizing buckets via views offer same precision so this statement is not fully true.
@@ -335,6 +335,61 @@ by using Views. | |||
|
|||
See [Program.cs](./Program.cs) for a complete example. | |||
|
|||
### Explicit Bucket Histogram Aggregation with .NET 9 Advice API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s misleading to call it .net 9 advice api as this api can be used in older .net runtimes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unresolved.
boundaries, consider the typical range of values that your application | ||
processes. This helps to ensure that the histogram provides relevant | ||
insights. | ||
- **Monitor and Adjust**: After implementing explicit bucket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without some concrete steps to monitor, I think it’s best to omit this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Annosha A lot of comments are marked resolved, but I don't see how. Please leave a comment about how the comment is addressed before resolving them.
@@ -335,6 +335,61 @@ by using Views. | |||
|
|||
See [Program.cs](./Program.cs) for a complete example. | |||
|
|||
### Explicit Bucket Histogram Aggregation with .NET 9 Advice API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thus document is about sdk customization, so histogram advice is somewhat misfit here. we can mention it in the views section about custom buckets, but anything more should be elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is unresolved.
@cijothomas @reyang @CodeBlanch please provide feedback on latest changes. |
@reyang please take a look at the improvements. |
Additionally, if an exponential histogram is defined using the View API, | ||
the explicit bucket boundaries provided by the Advice API will be ignored, | ||
as exponential histograms take precedence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to make sure I understand this - if the user leveraged the View API to choose a subset of the attributes, and there are no explicit buckets provided - would the buckets provided by the Hint API take effect or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the buckets provided by the Advice (Hint) API would take effect in this case. Here's what I understand:
-
Exponential Histogram with View API: If the user defines an exponential histogram via the View API, it takes full precedence, and any explicit bucket boundaries provided by the Advice API are ignored.
-
No Explicit Buckets Provided by View API: If the View API is used but does not define explicit bucket boundaries (e.g., it only filters attributes or configures other aspects), then:
-
The SDK will fall back to the explicit buckets from the Advice (Hint) API if they are available.
-
If no buckets are defined in the Advice API either, then the SDK defaults apply.
@cijothomas Could you please verify if these are correct?
Co-authored-by: Reiley Yang <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments left in the PR. My main concern is below:
Customizing the SDK section is not the right place to talk about Advice API in detail. We should mention it, and list the priority order when Advice and View are both used, but nothing more.
@@ -473,11 +572,11 @@ recording of `Exemplar`s. | |||
OpenTelemetry SDK comes with the following `ExemplarFilter`s (defined on | |||
`ExemplarFilterType`): | |||
|
|||
* (Default behavior) `AlwaysOff`: Makes no measurements eligible for becoming an | |||
- (Default behavior) `AlwaysOff`: Makes no measurements eligible for becoming an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove all unrelated changes from this PR. We are glad to accept them as a separate PR, so as to keep each PR focused on only one aspect only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good step forward. I feel we might need to change the structure of the doc, the current way how explicit buckets vs. base-2 exponential buckets are organized seems to be misleading to me. Not a blocker for this PR though.
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Co-authored-by: Reiley Yang <[email protected]>
Co-authored-by: Reiley Yang <[email protected]>
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Closes #5855 Update documentation for .NET 9 Advice API
Changes
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes