otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

fatsheep9146 · 2022-11-16T05:30:17Z

fixed #197
related #2840
Signed-off-by: Ziqi Zhao zhaoziqi9146@gmail.com

codecov · 2022-11-16T05:35:18Z

Codecov Report

Merging #3002 (256b144) into main (f44f5ad) will increase coverage by 0.1%.
The diff coverage is 95.3%.

@@           Coverage Diff           @@
##            main   #3002     +/-   ##
=======================================
+ Coverage   81.9%   82.1%   +0.1%     
=======================================
  Files        141     142      +1     
  Lines       9711    9818    +107     
=======================================
+ Hits        7963    8064    +101     
- Misses      1616    1619      +3     
- Partials     132     135      +3

Files Changed	Coverage
...n/google.golang.org/grpc/otelgrpc/stats_handler.go	`95.3%`

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

fatsheep9146 · 2023-02-10T08:52:04Z

@MrAlias @dashpole @dmathieu
Please help review this pr, thanks :)

dashpole · 2023-02-10T14:07:15Z

Thanks for tackling this!

I have a few questions. Can you update the description with the answers?

Is this meant to have feature parity with the existing otelgrpc instrumentation, and eventually replace it? Or is it meant to be something you use instead of otelgrpc instrumentation? If it is replacing it, what is the plan for deprecating the old one?
Is there anything you were unable to implement with the stats handler interface?

One other question:

Is it possible to run the same unit tests against a stats-handler-based implementation and the current implementation? Or did you copy existing test cases when writing these tests?

fatsheep9146 · 2023-03-20T05:14:15Z

Sorry for my delay

Is this meant to have feature parity with the existing otelgrpc instrumentation, and eventually replace it? Or is it meant to be something you use instead of otelgrpc instrumentation? If it is replacing it, what is the plan for deprecating the old one?

I think this feature is not to replace the existing one, but to provide a new way to instrument the grpc library. But in this new way with stats handler, we are able to implement more cases that the interceptors do not, for example, a failure to decode the message which is mentioned in #197 (comment).

Is there anything you were unable to implement with the stats handler interface?

For now, I think stats handler has much more info about the grpc request, response and connection, so it definitely is a good place to add instrument logic. I can not answer this question based on what I know about the grpc stats handler.

Is it possible to run the same unit tests against a stats-handler-based implementation and the current implementation? Or did you copy existing test cases when writing these tests?

Yes, I already add the same test like interceptors in test directory.

@dashpole

fatsheep9146 · 2023-03-20T05:23:26Z

@open-telemetry/go-approvers hi, is anyone able to review this pr? Thanks a lot

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

pellared

Leaving a quick review feedback

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

CHANGELOG.md

instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go

fatsheep9146 · 2023-09-18T10:37:25Z

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

pellared · 2023-09-18T10:54:18Z

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?

fatsheep9146 · 2023-09-18T11:33:59Z

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

Co-authored-by: Robert Pająk <pellared@hotmail.com>

fatsheep9146 · 2023-09-18T14:02:02Z

@pellared Thanks for creating the following issues!

hanyuancheung · 2023-09-19T05:09:42Z

It seems that this PR could be merged first, and then we could continue the following work.

pellared · 2023-09-19T06:09:10Z

@fatsheep9146 Merged 🎉 Thank you for your patience ❤️ You can follow-up with #4316 and #4317

fatsheep9146 · 2023-09-19T06:46:50Z

@fatsheep9146 Merged 🎉 Thank you for your patience ❤️ You can follow-up with #4316 and #4317

Thanks! I will solve the follow-up ASAP. @pellared

mehdihadeli · 2023-09-28T22:33:48Z

Hi,
When do you release new style of grpc otel? V0.44 doesn't have grpc metrics

dmathieu · 2023-09-29T07:11:48Z

0.45.0 was released a few hour ago.
https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.20.0

pellared · 2023-09-29T07:52:44Z

0.45.0 was released a few hour ago. https://github.com/open-telemetry/opentelemetry-go-contrib/releases/tag/v1.20.0

@mehdihadeli Yet metrics are not supported yet. See #4316. We hope to implement it for the next release.

This replaces the deprecated use of interceptors. See open-telemetry/opentelemetry-go-contrib#3002. Remove use of Goa request ID middleware. This is superseded by logging span IDs.

* Switch to using OpenTelemetry for both tracing and metrics This commit replaces the `metrics` and `trace` packages with a new `clue` package that leverages OpenTelemetry for instrumenting metrics and traces. See the README for instructions on how to upgrade. * Add tests * Remove coupling on Goa middleware * Use GRPC stat.handler for tracing in gRPC (#349) This replaces the deprecated use of interceptors. See open-telemetry/opentelemetry-go-contrib#3002. Remove use of Goa request ID middleware. This is superseded by logging span IDs. * Add example `tester` service to show API Integration Testing of a Goa System (#331) * initial commit * Empty-Commit * add readme file (still need to fill out) * tester readme update * Trying out a workflow to run integration tests against local binaries * Add proto install to /example/weather/scripts/setup * fix integration.yaml * disable mono on ubuntu * move mono stop * Try to find out what's running on 8084 * Trying to disable mono? * trying to stop mono 2 * add a sleep after stop/disable? * add some echo * see if stop/disable mono fails * add back in || true, change front ports to see if this even works at all * oops forgot to change int test port * try no send to dev null? * sleep before testing * take out netstat * sleep longer? give time for docker to do work to get grafana? * remove dev>null again... :\ * sleep 60 :( * Implement review comment changes * Fix runTests check for filtering if * Empty-Commit * try starting each service individually, check fo rmono * just KILL mono? * back to 8084, check all ports * fix typo * add flag to turn off grafana for services (for CI testing) * clean up integration.yaml script * review fixes * Get Stack Trace to Err Log on Panic * small fixes for style * protoc -> 25.1, better wait in integration.yaml * Add synchronous testing feature * Make one of the synchronous as an example * move func maps to service definition * Remove use of legacy flag --------- Co-authored-by: Jason Borneman <49321691+jasondborneman@users.noreply.github.com>

fatsheep9146 changed the title ~~[otlpgrpc] refactor otlpgrpc to use grpc.StatsHandler~~ [otelgrpc] refactor otelgrpc to use grpc.StatsHandler Nov 16, 2022

fatsheep9146 force-pushed the otelgrpc-use-statshandler branch 3 times, most recently from c08371f to 91dc29d Compare February 10, 2023 05:23

fatsheep9146 marked this pull request as ready for review February 10, 2023 05:40

fatsheep9146 requested review from jmacd, MrAlias, Aneurysm9, evantorrie, XSAM, dashpole, MadVikingGod, pellared, hanyuancheung and dmathieu as code owners February 10, 2023 05:40

hanyuancheung reviewed Feb 10, 2023

View reviewed changes

hanyuancheung requested a review from a team as a code owner March 5, 2023 12:31

jmacd mentioned this pull request Mar 15, 2023

[Exporters] Add bytes egressed metrics for exporters open-telemetry/opentelemetry-collector#7310

Open

fatsheep9146 force-pushed the otelgrpc-use-statshandler branch from 9cac2f9 to c992fdb Compare March 20, 2023 05:20

s-matyukevich reviewed Apr 11, 2023

View reviewed changes

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Show resolved Hide resolved

ash2k reviewed May 3, 2023

View reviewed changes

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Outdated Show resolved Hide resolved

fatsheep9146 force-pushed the otelgrpc-use-statshandler branch 2 times, most recently from 45a26ea to c5ec7ae Compare May 21, 2023 04:50

ash2k reviewed May 21, 2023

View reviewed changes

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Outdated Show resolved Hide resolved

pellared reviewed May 22, 2023

View reviewed changes

instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

instrumentation/google.golang.org/grpc/otelgrpc/test/stats_handler_test.go Outdated Show resolved Hide resolved

Update instrumentation/google.golang.org/grpc/otelgrpc/stats_handler.go

494eb36

Co-authored-by: Robert Pająk <pellared@hotmail.com>

fatsheep9146 requested a review from pellared September 18, 2023 12:58

This was referenced Sep 18, 2023

otelgro #4315

Closed

otelgrpc: Add metrics support for stats handler #4316

Closed

Update CHANGELOG.md

256b144

pellared approved these changes Sep 18, 2023

View reviewed changes

pellared changed the title ~~[otelgrpc] implement grpc.StatsHandler for grpc instrumentation~~ otelgrpc: Implement grpc.StatsHandler Sep 18, 2023

This was referenced Sep 18, 2023

otelgrpc: Update examples to use stats Handlers #4317

Closed

otelgrpc: Deprecate the interceptors #4318

Closed

otelgrpc: Remove the interceptors #4319

Open

dmathieu approved these changes Sep 18, 2023

View reviewed changes

pellared changed the title ~~otelgrpc: Implement grpc.StatsHandler~~ otelgrpc: Implement grpc.StatsHandler for trace instrumentation Sep 19, 2023

pellared merged commit bf86b26 into open-telemetry:main Sep 19, 2023
25 checks passed

MrAlias mentioned this pull request Sep 28, 2023

Release v1.20.0/v0.45.0/v0.14.0 #4351

Merged

pellared mentioned this pull request Nov 6, 2023

otelgrpc: Add peer attributes to spans generated by stats handlers #4531

Closed

raphael mentioned this pull request Jan 2, 2024

Use GRPC stat.handler for tracing in gRPC goadesign/clue#349

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

fatsheep9146 commented Nov 16, 2022 •

edited

codecov bot commented Nov 16, 2022 •

edited

fatsheep9146 commented Feb 10, 2023

dashpole commented Feb 10, 2023

fatsheep9146 commented Mar 20, 2023

fatsheep9146 commented Mar 20, 2023

pellared left a comment

fatsheep9146 commented Sep 18, 2023

pellared commented Sep 18, 2023 •

edited

fatsheep9146 commented Sep 18, 2023 •

edited

fatsheep9146 commented Sep 18, 2023

hanyuancheung commented Sep 19, 2023

pellared commented Sep 19, 2023

fatsheep9146 commented Sep 19, 2023

mehdihadeli commented Sep 28, 2023

dmathieu commented Sep 29, 2023

pellared commented Sep 29, 2023

otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

otelgrpc: Implement grpc.StatsHandler for trace instrumentation #3002

Conversation

fatsheep9146 commented Nov 16, 2022 • edited

codecov bot commented Nov 16, 2022 • edited

Codecov Report

fatsheep9146 commented Feb 10, 2023

dashpole commented Feb 10, 2023

fatsheep9146 commented Mar 20, 2023

fatsheep9146 commented Mar 20, 2023

pellared left a comment

Choose a reason for hiding this comment

fatsheep9146 commented Sep 18, 2023

pellared commented Sep 18, 2023 • edited

fatsheep9146 commented Sep 18, 2023 • edited

fatsheep9146 commented Sep 18, 2023

hanyuancheung commented Sep 19, 2023

pellared commented Sep 19, 2023

fatsheep9146 commented Sep 19, 2023

mehdihadeli commented Sep 28, 2023

dmathieu commented Sep 29, 2023

pellared commented Sep 29, 2023

fatsheep9146 commented Nov 16, 2022 •

edited

codecov bot commented Nov 16, 2022 •

edited

pellared commented Sep 18, 2023 •

edited

fatsheep9146 commented Sep 18, 2023 •

edited