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

Telemetry support #229

Closed
wingyplus opened this issue Jul 19, 2022 · 6 comments · Fixed by #298
Closed

Telemetry support #229

wingyplus opened this issue Jul 19, 2022 · 6 comments · Fixed by #298
Assignees
Milestone

Comments

@wingyplus
Copy link
Contributor

It would be good if have telemetry support for this library. I'm not sure if interceptor is a good fit but we can start from that.

@polvalente
Copy link
Contributor

I think some default interceptors would do just fine, although perhaps we can have events being published directly by the core server and client functions.

I would have something like [:grpc, :server, ...] and [:grpc, :client, ...] for the event names.
What do you propose we publish through telemetry? I think we should decide what events we want and then we can look at how to implement them

@wingyplus
Copy link
Contributor Author

wingyplus commented Jul 20, 2022

I want to start something simple like execution time such as [:grpc, :server, :start], [:grpc, :server, :stop], [:grpc, :client, :start] and [:grpc, :client, :stop]. (Event prefix can be change). This could use :telemetry.span to wrap this process.

I would prototype this feature to see how it going and discuss a bit more in details. :)

@polvalente
Copy link
Contributor

Those make sense, although I would use [:grpc, :server, :request] as the name of the event (to which telemetry span would append start and stop and exception).

I think we should include the stream as part of the metadata. I think it's basically a mixture of what Tesla and Phoenix do with Telemetry.

Since client-server connections aren't necessarily related to each request, we should perhaps add separate events for connect and disconnect on both sides as well

@polvalente
Copy link
Contributor

@wingyplus to be clear, I think we're ready for a PR on these events :)
Ideally we don't do it using the adapters so that we can ensure the events are published regardless of the adapter used

@wingyplus
Copy link
Contributor Author

Yeah. I’ll looking into it after setup ci is done. :)

@polvalente
Copy link
Contributor

As a point of reference, I'll work to publish events so we can export all metrics that grpc_prometheus currently exports

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

Successfully merging a pull request may close this issue.

2 participants