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

Port of jaeger remote sampler #2434

Conversation

sconover
Copy link

Depends on proposed changes to opentelementry python:
open-telemetry/opentelemetry-python#3852

Description

This is an early PR that proposes to add jaeger remote sampling support to the contrib repo.
Given that this is a port, there are multiple issues to discuss, from code style to overall approach to
properly integrating these samplers into the opentelemetry-python trace client lib.

Once all TODO's are resolved, in this PR and the PR it depends on, docs need to be added/updated.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Ported unit tests from jaeger-client-python
  • For lack of end-to-end tests, I put together a sample flask app and, running alongside a jaeger agent, manually verified that remote configuration changes indeed sync over to the client and change the client's sampling decision behavior.

Does This PR Require a Core Repo Change?

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project (I believe so)
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Depends on proposed changes to opentelementry python:
  open-telemetry/opentelemetry-python#3852
@aabmass
Copy link
Member

aabmass commented Apr 16, 2024

I hate to go back and forth on this, but shouldn't this be in the core repo @srikanthccv? It's a part of the official OTel SDK spec now https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/sdk.md#jaegerremotesampler.

@srikanthccv
Copy link
Member

srikanthccv commented Apr 16, 2024

I forgot that it was part of the official spec. I had seen go implementation in their contrib repo https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/samplers/jaegerremote. I apologize for the back and forth. The original PR in the core repo implemented it in the SDK package itself. It can be part of the core repo but I think it should be a separate package (similar to jaeger propagator).

@aabmass
Copy link
Member

aabmass commented Apr 17, 2024

It can be part of the core repo but I think it should be a separate package (similar to jaeger propagator).

👍 that sounds good to me. @sconover apologies for the miscommunication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants