Skip to content

Allow specifying samplers for the OpenTelemetry tracer via a new configuration.#30259

Merged
mattklein123 merged 11 commits intoenvoyproxy:mainfrom
dynatrace-oss-contrib:feat/sampler
Oct 30, 2023
Merged

Allow specifying samplers for the OpenTelemetry tracer via a new configuration.#30259
mattklein123 merged 11 commits intoenvoyproxy:mainfrom
dynatrace-oss-contrib:feat/sampler

Conversation

@samohte
Copy link
Copy Markdown
Contributor

@samohte samohte commented Oct 17, 2023 •
edited
Loading

Commit Message: Allow specifying samplers for the OpenTelemetry tracer via a new configuration. Add AlwaysOnSampler as reference implementation.
Additional Description: The OpenTelemetry project defines the concept of a Sampler API. This PR adds a generic interface to allow to implement a sampler which is used in the existing OpenTelemetry tracer. The change allows to add different samplers in the future, e.g. probabilistic sampler, vendor specific sampler, ..
An AlwaysOnSampler is added as reference implementation.
Risk Level: Low
Testing: multiple unit test, no functional
Docs Changes: Added OpenTelemetry Samplers to trace overview page (see change set)
Release Notes: N/A
Platform Specific Features: N/A
Fixes #29578

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 17, 2023

Hi @samohte, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #30259 was opened by samohte.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 17, 2023

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #30259 was opened by samohte.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 17, 2023

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #30259 was opened by samohte.

see: more, trace.

@samohte samohte force-pushed the feat/sampler branch 2 times, most recently from 75681b0 to 590eb71 Compare October 17, 2023 11:10
@samohte
Allow specifying samplers for the OpenTelemetry tracer via a new conf…
63a4be2 
…iguration. Add AlwaysOnSampler as reference implementation.

Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>

Signed-off-by: Thomas Ebner <96168670+samohte@users.noreply.github.com>
Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 17, 2023

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/30259/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #30259 (comment) was created by @samohte.

see: more, trace.

@samohte
Copy link
Copy Markdown
Contributor Author

samohte commented Oct 17, 2023 •
edited
Loading

Sorry for the huge PR. I tried to make the PR as small as possible, but introducing the API without at least one reference implementation does not make much sense. So, I added the API and the AlwaysOnSampler in this PR.

There is a unit test failing in the coverage step. But I think this is not related to my changes.

@samohte samohte marked this pull request as ready for review October 17, 2023 15:11
@samohte samohte requested a review from htuch as a code owner October 17, 2023 15:11
@samohte samohte mentioned this pull request Oct 17, 2023
Closed
@joaopgrassi joaopgrassi mentioned this pull request Oct 19, 2023
Closed
@joaopgrassi
Copy link
Copy Markdown
Contributor

joaopgrassi commented Oct 19, 2023 •
edited
Loading

@htuch Thomas works with me at Dynatrace, this is the initial/foundation Sampler addition we discussed in Slack.

mattklein123
mattklein123 requested changes Oct 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM. Can you add some type of integration test for this? Thank you.

/wait

@samohte
Merge remote-tracking branch 'upstream/main' into feat/sampler
4f5930b 
Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@samohte
add integration test, remove redundant method, do not set empty trace…
3bf0fb6 
…state

Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@samohte
Merge remote-tracking branch 'upstream/main' into feat/sampler
b47fd5b 
Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@samohte
Copy link
Copy Markdown
Contributor Author

samohte commented Oct 24, 2023 •
edited
Loading

Thanks LGTM. Can you add some type of integration test for this? Thank you.

/wait

Thanks for your feedback. I have added an integration test following https://github.com/envoyproxy/envoy/blob/main/test/integration/README.md. The test uses the opentelemetry tracer with the AlwaysOnSampler.

@samohte samohte requested a review from mattklein123 October 24, 2023 12:02
samohte and others added 3 commits October 25, 2023 11:54
@samohte
Merge remote-tracking branch 'upstream/main' into feat/sampler
6256cf3 
Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@samohte
Merge remote-tracking branch 'upstream/main' into feat/sampler
d800b87 
Signed-off-by: thomas.ebner <thomas.ebner@dynatrace.com>
@joaopgrassi
Merge remote-tracking branch 'upstream/main' into feat/sampler
6993541 
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@joaopgrassi
Copy link
Copy Markdown
Contributor

joaopgrassi commented Oct 27, 2023

@mattklein123 hi! We pushed an integration test. Should be good for another look. Thank you!

@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Oct 27, 2023

Can you merge main please?

/wait

@joaopgrassi
Merge remote-tracking branch 'upstream/main' into feat/sampler
e56a5d6 
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@joaopgrassi
Fix linter
3a22ec2 
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@joaopgrassi
Copy link
Copy Markdown
Contributor

joaopgrassi commented Oct 30, 2023

Can you merge main please?

@mattklein123 done!

@joaopgrassi
Proto format/lint
a0c1102 
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
@joaopgrassi
Merge remote-tracking branch 'upstream/main' into feat/sampler
e604134 
Signed-off-by: Joao Grassi <joao.grassi@dynatrace.com>
mattklein123
mattklein123 approved these changes Oct 30, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit c600637 into envoyproxy:main Oct 30, 2023
This was referenced Nov 9, 2023
Closed
Merged
Merged
@joaopgrassi joaopgrassi mentioned this pull request Feb 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@htuch htuch Awaiting requested review from htuch

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

OpenTelemetry tracer - Hability to configure a custom sampler

3 participants

@samohte @joaopgrassi @mattklein123