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

otelgrpc: Add metrics support for stats handler #4316

Closed
pellared opened this issue Sep 18, 2023 · 5 comments · Fixed by #4356
Closed

otelgrpc: Add metrics support for stats handler #4316

pellared opened this issue Sep 18, 2023 · 5 comments · Fixed by #4356
Assignees
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgrpc
Milestone

Comments

@pellared
Copy link
Member

I noticed that the stats handler misses the functionality you added here: #2700. Any reasons for that?

I was plan to implement the stats.handler step by step, cover the trace first, and then cover the metrics. Which is also convenient for review.
Do you think I should implement it in this pr or create another pr for metrics?
I'm open for both way. @pellared

During the SIG meetings we discussed that we would like to have a feature-parity before we deprecate interceptors as part of #3002 (review).

Still I think we can tackle it in a separate PR.

Are there any other features which are missing when comparing with the interceptors?

I think metric is the only missing feature, since I wrote a simple e2e test TestStatsHandler just like interceptors to make sure the trace related function is complete.
@pellared

Originally posted by @fatsheep9146 in #3002 (comment)

@pellared pellared changed the title otelgr otelgrpc: Add metrics support for stats handler Sep 18, 2023
@pellared pellared mentioned this issue Sep 18, 2023
@pellared pellared added this to the v0.45.0 milestone Sep 18, 2023
@pellared pellared added instrumentation: otelgrpc enhancement New feature or request area: instrumentation Related to an instrumentation package labels Sep 18, 2023
@pellared
Copy link
Member Author

@fatsheep9146 Can you take this?

@pellared
Copy link
Member Author

It would be good to solve #4323 before this or as part of this issue.

@fatsheep9146
Copy link
Contributor

It would be good to solve #4323 before this or as part of this issue.

Ok, I will try to fix #4323, if no one is working on this.

@Sovietaced
Copy link
Contributor

Ok, I will try to fix #4323, if no one is working on this.

I'm happy to work on this if it means implementing an rcpcServerDuration histogram alongside the existing tracing code.

@fatsheep9146
Copy link
Contributor

Ok, I will try to fix #4323, if no one is working on this.

I'm happy to work on this if it means implementing an rcpcServerDuration histogram alongside the existing tracing code.

I'm already working on this issue now, and before this work is merged, the #4323 should also be fixed, I'm now fixing the #4323 first. @Sovietaced

I think you could help review and add more meaningful metric after this issue is done if you want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: instrumentation Related to an instrumentation package enhancement New feature or request instrumentation: otelgrpc
Projects
None yet
4 participants