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.

Sign up for GitHub

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

Jump to bottom

Tracing: Support remote, rate-limited, and probabilistic sampling in tracing.opentelemetry config section #73587

Merged
merged 11 commits into from
Sep 11, 2023
Merged

Tracing: Support remote, rate-limited, and probabilistic sampling in tracing.opentelemetry config section #73587

merged 11 commits into from
Sep 11, 2023

Conversation

hairyhenderson
Copy link
Member

What is this feature?

Supports sampling configuration in the tracing.opentelemetry config section. All sampling strategies that were supported with Jaeger are supported now with all OTel providers (Jaeger & OTLP), especially the remote sampling server.

Why do we need this feature?

The tracing.jaeger section is deprecated, but there's no way to configure sampling otherwise.

Who is this feature for?

Operators

Which issue(s) does this PR fix?:

Fixes #43947

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@hairyhenderson hairyhenderson self-assigned this Aug 21, 2023
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.2.x milestone Aug 21, 2023
@hairyhenderson hairyhenderson force-pushed the add-otel-remote-sampling-support-43947 branch from 2f81a75 to ea141da Compare August 21, 2023 21:10
@hairyhenderson hairyhenderson marked this pull request as ready for review August 21, 2023 21:11
@hairyhenderson hairyhenderson requested review from marefr, oshirohugo, papagian, zserge and mildwonkey and removed request for a team August 21, 2023 21:11
chri2547
chri2547 requested changes Aug 22, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

@hairyhenderson , added some suggestions and a couple of questions.

@xnyo xnyo self-requested a review August 28, 2023 14:35
chri2547
chri2547 approved these changes Sep 8, 2023
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the contribution!

marefr
marefr approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@marefr marefr left a comment

Choose a reason for hiding this comment

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

pkg/plugins/localfiles.go change is fine. Other changes LGTM as well, but would like @grafana/backend-platform to have a look as well since they're codeowners

hairyhenderson and others added 10 commits September 11, 2023 11:41
@hairyhenderson
tracing: Support remote sampling server
1f9ac6c 
Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
@hairyhenderson @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
e163177 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@hairyhenderson @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
24b93e2 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@hairyhenderson @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
6959f9c 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@hairyhenderson @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
3743014 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
3b366e4
@hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
9587627
@hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
4563b4d
@hairyhenderson
Satisfying the doc-validator check
92acc78
@hairyhenderson
satisfy prettier
ead7d74 
Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
@hairyhenderson hairyhenderson force-pushed the add-otel-remote-sampling-support-43947 branch from 51c3956 to ead7d74 Compare September 11, 2023 15:41
xnyo
xnyo approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@xnyo xnyo left a comment

Choose a reason for hiding this comment

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

Left some nits but LGTM!

This will probably also require changes in the plugin-sdk so the sampler settings are propagated properly:

https://github.com/grafana/grafana-plugin-sdk-go/blob/05a3321740553735a6181e1491a1a02547973a7f/internal/tracerprovider/tracerprovider.go#L72

But I can open an issue and do that separately :)

pkg/plugins/localfiles.go Outdated Show resolved Hide resolved
pkg/plugins/localfiles.go Outdated Show resolved Hide resolved
@hairyhenderson
back out unnecessary change
6456011 
Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
@hairyhenderson
Copy link
Member Author

This will probably also require changes in the plugin-sdk so the sampler settings are propagated properly:

https://github.com/grafana/grafana-plugin-sdk-go/blob/05a3321740553735a6181e1491a1a02547973a7f/internal/tracerprovider/tracerprovider.go#L72

But I can open an issue and do that separately :)

@xnyo ah - thanks for that.

@hairyhenderson hairyhenderson merged commit ce1169f into main Sep 11, 2023
14 checks passed
@hairyhenderson hairyhenderson deleted the add-otel-remote-sampling-support-43947 branch September 11, 2023 16:13
@hairyhenderson hairyhenderson mentioned this pull request Sep 11, 2023
Closed
3 tasks
@xnyo xnyo mentioned this pull request Sep 12, 2023
Closed
chauchausoup pushed a commit to chauchausoup/grafana that referenced this pull request Sep 15, 2023
@hairyhenderson @chri2547 @chauchausoup
Tracing: Support remote, rate-limited, and probabilistic sampling in …
f63dbfc 
…tracing.opentelemetry config section (grafana#73587)

* tracing: Support remote sampling server

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Satisfying the doc-validator check

* satisfy prettier

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

* back out unnecessary change

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

---------

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
rwwiv pushed a commit that referenced this pull request Oct 2, 2023
@hairyhenderson @chri2547 @rwwiv
Tracing: Support remote, rate-limited, and probabilistic sampling in …
b189794 
…tracing.opentelemetry config section (#73587)

* tracing: Support remote sampling server

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Update docs/sources/setup-grafana/configure-grafana/_index.md

* Satisfying the doc-validator check

* satisfy prettier

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

* back out unnecessary change

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>

---------

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@zerok zerok modified the milestones: 10.2.x, 10.2.0 Oct 23, 2023
@dnhn dnhn mentioned this pull request Oct 24, 2023
Closed
@BrewTestBot BrewTestBot mentioned this pull request Oct 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chri2547 chri2547 chri2547 approved these changes

@github-actions github-actions[bot] github-actions left review comments

@marefr marefr marefr approved these changes

@xnyo xnyo xnyo approved these changes

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

@oshirohugo oshirohugo Awaiting requested review from oshirohugo oshirohugo was automatically assigned from grafana/plugins-platform-backend

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@mildwonkey mildwonkey Awaiting requested review from mildwonkey mildwonkey was automatically assigned from grafana/backend-platform

Assignees

@hairyhenderson hairyhenderson

Labels
add to changelog area/backend no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
10.2.0
Development

Successfully merging this pull request may close these issues.

Extend configurability of OpenTelemetry
5 participants
@hairyhenderson @marefr @xnyo @chri2547 @zerok