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

Switch Jaeger remote sampler to "grpc" lite mechanism #4014

Closed
pavolloffay opened this issue Dec 20, 2021 · 4 comments · Fixed by #4043
Closed

Switch Jaeger remote sampler to "grpc" lite mechanism #4014

pavolloffay opened this issue Dec 20, 2021 · 4 comments · Fixed by #4043
Labels
Feature Request Suggest an idea for this project

Comments

@pavolloffay
Copy link
Member

Is your feature request related to a problem? Please describe.

This is required for open-telemetry/opentelemetry-java-instrumentation#4898 (comment)

I am starting to work on this

@pavolloffay pavolloffay added the Feature Request Suggest an idea for this project label Dec 20, 2021
@pavolloffay
Copy link
Member Author

@anuraaga the remote sampler needs to get the response from the grpc service. So far the grpc lite mechanism is not parsing the response e.g. -

Any preferences how it should be done? I have started by defining public interface GrpcService<REQ extends Marshaler, RES extends Marshaler> I don't have it working yet.

@anuraaga
Copy link
Contributor

anuraaga commented Dec 21, 2021

@pavolloffay GrpcExporter is an abstraction for exporting, but the actual stub is classes like these

https://github.com/open-telemetry/opentelemetry-java/blob/main/exporters/otlp/trace/src/main/java/io/opentelemetry/exporter/otlp/trace/MarshalerTraceServiceGrpc.java#L43

For the remote sampler, I don't think we need an additional abstraction, we should be able to use that type of class for the gRPC stub, and actually implement the RESPONSE_MARSHALER instead of returning a singleton.

@pavolloffay
Copy link
Member Author

The issue is that we need logic from

and {OkHttp,Default}GrpcExporterBuilder to make it all work.

an abstraction that would handle generic response type would be helpful.

@anuraaga
Copy link
Contributor

I see - in that case the interface you mentioned does seem like a reasonable direction. I don't expect to need it outside the Jaeger sampler artifact though so if there is a solution that keeps things simpler without abstracting more than needed, we don't need to consider anything very general.

FWIW, I think we could consider making setChannel deprecated and a no-op, only having one implementation, unless we later got a complaint. I have a feeling in practice it's never used for the Jaeger sampler. But it is a bit risky, I would personally approve a PR that did that if you are up to it.

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

Successfully merging a pull request may close this issue.

2 participants