search

elixir-grpc / grpc

An Elixir implementation of gRPC
https://hex.pm/packages/grpc
Apache License 2.0
1.38k stars 212 forks source link

Add support for OpenTelemetry interceptor #264

Open carrascoacd opened 2 years ago

carrascoacd commented 2 years ago

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 commented 2 years ago

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

polvalente commented 2 years ago

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

polvalente commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 9 months ago

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

polvalente commented 9 months ago

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