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

Add support for OpenTelemetry interceptor #264

Open
carrascoacd opened this issue Aug 11, 2022 · 13 comments
Open

Add support for OpenTelemetry interceptor #264

carrascoacd opened this issue Aug 11, 2022 · 13 comments

Comments

@carrascoacd
Copy link
Contributor

carrascoacd commented Aug 11, 2022

Context

In order to support OpenTelemetry traces with gRPC, as I didn't find any available solution across the community, I propose creating an interceptor for that purpose.

Idea

The interceptor is quite simple, I managed to get something working based on opentelemetry_tesla. I can work on it and open a PR with that job once we agree on it.

Please, let me know @polvalente if it makes sense, and if it makes sense to create a new interceptor inside this library or if you prefer instead to work on a separate repository.

@sleipnir
Copy link
Collaborator

Guys, I need to ask how this functionality affects the https://github.com/elixir-grpc/grpc-prometheus project?

@polvalente
Copy link
Contributor

@sleipnir one shouldn't affect the other. What are your concerns?

@polvalente
Copy link
Contributor

Also, we can decide on having a separate lib like grpc-prometheus, for instance grpc-opentelemetry

However, I don't have the access to create it, so we'd have to check in with the other maintainers.

@sleipnir
Copy link
Collaborator

grpc-opentelemetry

The issue is to ensure that telemetry data is also available for Prometheus. This allows seamless integration with kubernetes and grafana stack

@polvalente
Copy link
Contributor

grpc-opentelemetry

The issue is to ensure that telemetry data is also available for Prometheus. This allows seamless integration with kubernetes and grafana stack

One doesn't affect the other :)

@carrascoacd
Copy link
Contributor Author

carrascoacd commented Aug 12, 2022

Also, we can decide on having a separate lib like grpc-prometheus, for instance grpc-opentelemetry

I think it makes sense to create a separate lib, among other reasons, because this is like the convention when defining third-party integrations with OTel and also because the testing will be isolated so we don't have to mess this library with the erlang-telemetry dependencies.

@polvalente
Copy link
Contributor

Also, we can decide on having a separate lib like grpc-prometheus, for instance grpc-opentelemetry

I think it makes sense to create a separate lib, among other reasons, because this is like the convention when defining third-party integrations with OTel and also because the testing will be isolated so we don't have to mess this library with the erlang-telemetry dependencies.

There's another approach, though, which is to make opentelemetry an optional dependency and only compile the related modules if it is present.

I'll see if we can create a new lib for this.

@wingyplus
Copy link
Contributor

wingyplus commented Sep 22, 2022

I'm working on the interceptor library here. It focuses on tracing and client-side first which's my primary usage.

it's not production ready at the moment. So please use it with caution and give feedback. PR is very welcome. :)

@carrascoacd
Copy link
Contributor Author

Thanks for sharing! @wingyplus, I'm about to test the propagator I mentioned I was working on in production soon. I've created a Gist with the content since it is still in our internal project. I think it is quite similar https://gist.github.com/carrascoacd/d80cdaf590d5fe14e75f95a2c91b3ede

I was waiting to contribute to the new library instead, that's why I didn't create a lib yet.

@wingyplus
Copy link
Contributor

Thanks for sharing! @wingyplus, I'm about to test the propagator I mentioned I was working on in production soon. I've created a Gist with the content since it is still in our internal project. I think it is quite similar https://gist.github.com/carrascoacd/d80cdaf590d5fe14e75f95a2c91b3ede

I was waiting to contribute to the new library instead, that's why I didn't create a lib yet.

Thanks. From a quick look, your implementation dont need to merge the header before put_header since that function will merge before replacing it.

Ref: https://github.com/elixir-grpc/grpc/blob/master/lib/grpc/client/stream.ex

I’ll take a deep look tonight to see what I miss. 🙇

@wingyplus
Copy link
Contributor

@carrascoacd I think the library cover all that you want. The main difference is that I use attributes following this guide https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/rpc.md. It might not cover all from the document but I'll try putting it as much as I can. :)

@davydog187
Copy link
Collaborator

I have implemented Otel instrumentation for client events that I can push up to the opentelemetry-erlang-contrib repo.

@polvalente
Copy link
Contributor

We could point to it in the README or on the Telemetry module in that case!

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

No branches or pull requests

5 participants