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

Add Sampling configuration page #14856

Merged
merged 10 commits into from May 8, 2024

Conversation

joaopgrassi
Copy link
Member

Description

Fixes #14831

Adds a new 'task' page that describes all the ways trace sampling can be configured.

Reviewers

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure
  • Localization/Translation

@istio-testing
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 8, 2024
@istio-testing istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 8, 2024
@joaopgrassi
Copy link
Member Author

CC @howardjohn @zirain

@joaopgrassi
Copy link
Member Author

/test all

@joaopgrassi joaopgrassi marked this pull request as ready for review April 11, 2024 10:56
@joaopgrassi joaopgrassi requested a review from a team as a code owner April 11, 2024 10:56
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Apr 11, 2024
Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM, agree would be nice to have a test if possible.

@joaopgrassi
Copy link
Member Author

Yeah I'm not sure about what to do with the tests. The snippets where we use the MeshConfig to set the percentage sampling or the Telemetry API can be tested but as I said above, it's just starting Istio. Not much we can test regarding the rest.

For the snippet that uses a custom sampler (The only now is the Dynatrace sampler) also can't be really tested as spans are exported to a Dynatrace environment and I doubt we want that in the tests.

So I'm not sure there's much value in it. But if it's required, then I can add some, to at least make sure Istio starts.

@joaopgrassi
Copy link
Member Author

@dhawton @linsun I added now tests for configuring percentage sampling via meshConfig and via telemetry API. For the "custom sampler" example, I can't as for that I'd need to deploy a ServiceEntry for the Dynatrace API, and I doubt we want that in a test.

@joaopgrassi
Copy link
Member Author

Hi @dhawton @linsun friendly ping on this one.

@kfaseela
Copy link
Member

cc @zirain

Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM thank you for adding the tests!

@joaopgrassi
Copy link
Member Author

Hi all, sorry for the ping, but is there anything else to do here before we merge?

@joaopgrassi
Copy link
Member Author

/retest

@joaopgrassi joaopgrassi requested a review from kfaseela May 7, 2024 14:10
@kfaseela
Copy link
Member

kfaseela commented May 8, 2024

LGTM.. you can just take care of Daniel's comments, and we are good to go

@joaopgrassi
Copy link
Member Author

LGTM.. you can just take care of Daniel's comments, and we are good to go

@kfaseela Done! thank you!

@joaopgrassi
Copy link
Member Author

/retest

Copy link
Member

@dhawton dhawton left a comment

Choose a reason for hiding this comment

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

LGTM as well.

@istio-testing istio-testing merged commit 0b35b5a into istio:master May 8, 2024
12 checks passed
@joaopgrassi joaopgrassi deleted the otel-sampler branch May 13, 2024 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/extensions and telemetry size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document ways to configure trace sampling
7 participants