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

Metrics customization guide with .NET 9 Advice API details #5911

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Annosha
Copy link

@Annosha Annosha commented Oct 21, 2024

Closes #5855 Update documentation for .NET 9 Advice API

Changes

  • Added documentation for explicit bucket histogram aggregation using the .NET 9 Advice API.
  • Provided example code to demonstrate the usage of explicit bucket boundaries in histograms.
  • Included best practices for selecting meaningful bucket boundaries and tips for using the new API.

Merge requirement checklist

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

@Annosha Annosha requested a review from a team as a code owner October 21, 2024 07:38
@github-actions github-actions bot added the documentation Documentation related label Oct 21, 2024
@Annosha
Copy link
Author

Annosha commented Oct 21, 2024

@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.
Copy link
Member

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
Copy link
Member

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

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

this is unresolved.

@Annosha
Copy link
Author

Annosha commented Oct 23, 2024

@cijothomas @reyang @CodeBlanch please provide feedback on latest changes.

@Annosha
Copy link
Author

Annosha commented Oct 24, 2024

@reyang please take a look at the improvements.

Comment on lines +246 to +248
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.
Copy link
Member

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?

Copy link
Author

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:

  1. 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.

  2. 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?

Copy link
Member

@cijothomas cijothomas left a 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
Copy link
Member

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.

Copy link
Member

@reyang reyang left a 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.

Copy link
Contributor

github-actions bot commented Nov 6, 2024

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.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 6, 2024
@github-actions github-actions bot removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 9, 2024
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation related Stale Issues and pull requests which have been flagged for closing due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update documentation for .NET 9 Advice API
4 participants