-
Notifications
You must be signed in to change notification settings - Fork 587
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 Jaeger remote sampler #936
Conversation
@pavolloffay were/are you working on this? |
For java not for golang |
@kvrhdn What's the status of this? Anything I could help with? I was looking to implement this myself and stumbled upon this PR. |
@joe-elliott fyi |
Honestly, I kind of lost track of this PR as my priorities changed. Checking my work again this is in pretty okay spot I think. IIRC I just wanted to implement the rate limiting strategy before opening it up for review, but how the sampler is supposed to work was a bit unclear (there is no documentation on each strategy AFAIK). I see I still have pending comments with questions that are 3 months old now lol 🙃 I can spend some time tomorrow to get this PR updated and open it for reviews. This should allows us to move forward with this sampler. |
These problems you mentioned should have been fixed now. Please run CI again. About flow control, I think the original author refers to your scheme. I don't think much about how to optimize it here. Please help cr. if there is no problem, please approve the integration and release a new version. This version is very important to me. |
Can you label it for me, because if you don't label it, CI can't get through. But I don't have this permission to tag. Thank you very much! go.opentelemetry.io/contrib/samplers/jaegerremote/v0.22.0 |
You need to add replace statements to cause it to use a local copy of the module. See other examples for examples. |
I did this before, but other students asked me to modify it. Now that I've changed it, please run CI. Thank you |
…78, but there are other ports specified by the user, so the sampling address and fetch address are strongly bound
I would like to ask when this PR can be integrated and released? I need it urgently. Thank you. |
I approved it, but I'm not a maintainer of this repo. |
Thank you very much. By the way, I also want to ask whether otel can support W3C and Zipkin's B3 propagation at the same time. What should I do? |
Style guide conformity concerns have been addressed.
Thank you @kvrhdn 🎉 |
🥳 🥳 🥳 |
Thank you very much. |
Add an implemention of the Jaeger remote sampler as described in the SDK spec, added by open-telemetry/opentelemetry-specification#1791.
Not yet implemented:
OTEL_TRACES_SAMPLER_ARG
environment variableCloses open-telemetry/opentelemetry-go#2148
Closes open-telemetry/opentelemetry-go#327