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

Jump to bottom

Add Jaeger remote sampler #936

Merged
merged 43 commits into from Mar 11, 2022
Merged

Add Jaeger remote sampler #936

merged 43 commits into from Mar 11, 2022

Conversation

kvrhdn
Copy link
Contributor

@kvrhdn kvrhdn commented Aug 4, 2021 •
edited

Add an implemention of the Jaeger remote sampler as described in the SDK spec, added by open-telemetry/opentelemetry-specification#1791.

Not yet implemented:

  • the rate limiting strategy
  • logic to parse the OTEL_TRACES_SAMPLER_ARG environment variable

Closes open-telemetry/opentelemetry-go#2148
Closes open-telemetry/opentelemetry-go#327

@kvrhdn kvrhdn mentioned this pull request Aug 4, 2021
Closed
kvrhdn
kvrhdn commented Aug 4, 2021
View reviewed changes
samplers/jaeger_remote/jaeger_remote.go Outdated Show resolved Hide resolved
samplers/jaeger_remote/jaeger_remote.go Outdated Show resolved Hide resolved
@joe-elliott
Copy link

joe-elliott commented Aug 9, 2021 •
edited

@pavolloffay were/are you working on this?

@pavolloffay
Copy link
Member

For java not for golang

@kvrhdn kvrhdn mentioned this pull request Aug 16, 2021
Closed
@matej-g
Copy link
Contributor

matej-g commented Nov 1, 2021

@kvrhdn What's the status of this? Anything I could help with? I was looking to implement this myself and stumbled upon this PR.

@yurishkuro
Copy link
Member

@joe-elliott fyi

@kvrhdn
Copy link
Contributor Author

kvrhdn commented Nov 1, 2021

@kvrhdn What's the status of this? Anything I could help with? I was looking to implement this myself and stumbled upon this PR.

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.

@kvrhdn kvrhdn marked this pull request as ready for review November 2, 2021 00:27
@dino-ma
Merge branch 'jaeger_remote' of github.com:kvrhdn/opentelemetry-go-co…
d4a19ac 
…ntrib into jaeger_remote

# Conflicts:
#	samplers/jaegerremote/sampler_remote_options.go
@dino-ma
Copy link
Contributor

dino-ma commented Mar 8, 2022

These problems you mentioned should have been fixed now. Please run CI again.
Thank you for your guidance.

About flow control, I think the original author refers to your scheme. I don't think much about how to optimize it here.
https://github.com/jaegertracing/jaeger-client-go/blob/master/utils/rate_limiter.go

@yurishkuro

Please help cr. if there is no problem, please approve the integration and release a new version. This version is very important to me.
@dashpole @MadVikingGod @pellared @MrAlias @evantorrie @paivagustavo @XSAM

@dino-ma
Copy link
Contributor

dino-ma commented Mar 9, 2022

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

https://github.com/open-telemetry/opentelemetry-go-contrib/runs/5465487196?check_suite_focus=true#step:7:458

@yurishkuro

@Aneurysm9
Copy link
Member

You need to add replace statements to cause it to use a local copy of the module. See other examples for examples.

@dino-ma
Copy link
Contributor

dino-ma commented Mar 9, 2022

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

@Aneurysm9

Aneurysm9 and others added 4 commits March 9, 2022 01:16
@Aneurysm9
Merge branch 'main' into jaeger_remote
bbb1c21
@dino-ma
fix lint
3fc124f
@dino-ma
make precommit fix
ecc557d
@dino-ma
(fix:sampler_remote_option.go):The default port of jaeger agent is 57…
139c1c8 
…78, but there are other ports specified by the user, so the sampling address and fetch address are strongly bound
@dino-ma
Copy link
Contributor

dino-ma commented Mar 9, 2022

I would like to ask when this PR can be integrated and released? I need it urgently. Thank you.
@yurishkuro

@yurishkuro
Copy link
Member

I approved it, but I'm not a maintainer of this repo.

@dino-ma
Copy link
Contributor

dino-ma commented Mar 10, 2022

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?

Aneurysm9
Aneurysm9 reviewed Mar 10, 2022
View reviewed changes
samplers/jaegerremote/doc.go Outdated Show resolved Hide resolved
samplers/jaegerremote/jaeger-idl/proto/api_v2/sampling.proto Show resolved Hide resolved
samplers/jaegerremote/sampler.go Outdated Show resolved Hide resolved
samplers/jaegerremote/sampler_remote_options.go Show resolved Hide resolved
@Aneurysm9 Aneurysm9 dismissed their stale review March 10, 2022 05:16

Style guide conformity concerns have been addressed.

@Aneurysm9 Aneurysm9 merged commit a700e18 into open-telemetry:main Mar 11, 2022
@jmacd
Copy link
Contributor

jmacd commented Mar 11, 2022

Thank you @kvrhdn 🎉

@yurishkuro
Copy link
Member

🥳 🥳 🥳

@dino-ma
Copy link
Contributor

dino-ma commented Mar 12, 2022

Thank you very much.

This was referenced Mar 16, 2022
Merged
Closed
@yurishkuro yurishkuro mentioned this pull request Oct 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro approved these changes

@vuuihc vuuihc vuuihc left review comments

@dino-ma dino-ma dino-ma left review comments

@matej-g matej-g matej-g left review comments

@Aneurysm9 Aneurysm9 Aneurysm9 approved these changes

@jmacd jmacd jmacd approved these changes

@hanyuancheung hanyuancheung hanyuancheung approved these changes

@dashpole dashpole Awaiting requested review from dashpole

@evantorrie evantorrie Awaiting requested review from evantorrie

@MadVikingGod MadVikingGod Awaiting requested review from MadVikingGod

@MrAlias MrAlias Awaiting requested review from MrAlias

@paivagustavo paivagustavo Awaiting requested review from paivagustavo

@pellared pellared Awaiting requested review from pellared

@XSAM XSAM Awaiting requested review from XSAM

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

New sampler: Jaeger remote Support for Jaeger remote sampler