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 metric rpc.server.duration support #2700

Merged

Conversation

fatsheep9146
Copy link
Contributor

Signed-off-by: Ziqi Zhao zhaoziqi9146@gmail.com

Enhance instrumentation library otelgrpc for grpc to satisfy opentelemetry grpc server metric semantic conventions.

This pr is meant to add metric rpc.server.duration for the first step.

I use example code to test, and produce the data for unary and stream request

{
    "Name": "rpc.server.duration{service.name=unknown_service:main,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc,instrumentation.version=semver:0.34.0,net.peer.ip=127.0.0.1,net.peer.port=55809,rpc.grpc.status_code=0,rpc.method=SayHelloServerStream,rpc.service=api.HelloService,rpc.system=grpc}",
    "Sum": 502.366387
},
{
    "Name": "rpc.server.duration{service.name=unknown_service:main,telemetry.sdk.language=go,telemetry.sdk.name=opentelemetry,telemetry.sdk.version=1.9.0,instrumentation.name=go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc,instrumentation.version=semver:0.34.0,net.peer.ip=127.0.0.1,net.peer.port=55809,rpc.grpc.status_code=0,rpc.method=SayHelloBidiStream,rpc.service=api.HelloService,rpc.system=grpc}",
    "Sum": 253.394227
}

@dmathieu
Copy link
Member

You need to run go mod tidy from the otelgrpc package to fix the tests.
Also, this will need a changelog entry.

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #2700 (41f7fb7) into main (bba2c55) will increase coverage by 0.0%.
The diff coverage is 63.6%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2700   +/-   ##
=====================================
  Coverage   69.3%   69.4%           
=====================================
  Files        145     146    +1     
  Lines       6729    6752   +23     
=====================================
+ Hits        4667    4689   +22     
+ Misses      1948    1946    -2     
- Partials     114     117    +3     
Impacted Files Coverage Δ
...ogle.golang.org/grpc/otelgrpc/metadata_supplier.go 51.1% <51.1%> (ø)
...entation/google.golang.org/grpc/otelgrpc/config.go 65.1% <80.0%> (ø)
...ion/google.golang.org/grpc/otelgrpc/interceptor.go 83.0% <100.0%> (-0.6%) ⬇️

@fatsheep9146 fatsheep9146 force-pushed the grpc-metric-rpc-server-duration branch from a6e086f to 561384b Compare August 31, 2022 11:15
@fatsheep9146
Copy link
Contributor Author

@dmathieu @Aneurysm9 All checks are passed, could you help review this again?

@fatsheep9146
Copy link
Contributor Author

Could you help review this again? @dmathieu

@fatsheep9146
Copy link
Contributor Author

@Aneurysm9 @jmacd @MrAlias @hanyuancheung

Could you help review this again?

@devnev
Copy link

devnev commented Sep 11, 2022

Hi, stumbled across this, and had two questions about the units

  • Why milliseconds? Looks like its a float64 anyway, so the position of the decimal makes no technical difference, so seconds would seem like a better choice simply for being the SI standard. nevermind, spotted the unit specification in the upstream spec.
  • Does the unit need to be indicated somewhere? I had to dig into the recording code to find it.

@fatsheep9146
Copy link
Contributor Author

fatsheep9146 commented Sep 13, 2022

@devnev
I think you can visit here to find the info about all metrics.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/rpc.md#rpc-server

Hi, stumbled across this, and had two questions about the units

  • Why milliseconds? Looks like its a float64 anyway, so the position of the decimal makes no technical difference, so seconds would seem like a better choice simply for being the SI standard. nevermind, spotted the unit specification in the upstream spec.
  • Does the unit need to be indicated somewhere? I had to dig into the recording code to find it.

@fatsheep9146 fatsheep9146 force-pushed the grpc-metric-rpc-server-duration branch 3 times, most recently from 561b10b to 9e3f735 Compare September 19, 2022 05:08
@fatsheep9146
Copy link
Contributor Author

ping @pellared @Aneurysm9 @MrAlias @jmacd @dashpole @MadVikingGod @evantorrie @XSAM , could you help review this again?

@fatsheep9146
Copy link
Contributor Author

ping @pellared @Aneurysm9 @MrAlias @jmacd @dashpole @MadVikingGod @evantorrie @XSAM could you help review this?

@MrAlias
Copy link
Contributor

MrAlias commented Oct 4, 2022

Is the plan to add the other specified metrics (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/rpc.md#rpc-server) in follow on PRs? If so, can you open an issue to track the work?

@fatsheep9146
Copy link
Contributor Author

Is the plan to add the other specified metrics (https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/semantic_conventions/rpc.md#rpc-server) in follow on PRs? If so, can you open an issue to track the work?

Absolutely, I already open an issue #2840 to track, and I will add the metric one by one. @MrAlias

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the instrumentation readme to indicate this package supports metric exports: https://github.com/open-telemetry/opentelemetry-go-contrib/tree/main/instrumentation#instrumentation-packages

CHANGELOG.md Outdated Show resolved Hide resolved
instrumentation/google.golang.org/grpc/otelgrpc/config.go Outdated Show resolved Hide resolved
@fatsheep9146
Copy link
Contributor Author

Could you help review this again? @MrAlias

Signed-off-by: Ziqi Zhao <zhaoziqi9146@gmail.com>
@fatsheep9146 fatsheep9146 force-pushed the grpc-metric-rpc-server-duration branch from 8041c7b to c91c855 Compare October 23, 2022 04:47
@fatsheep9146
Copy link
Contributor Author

@jmacd @pellared @Aneurysm9 Could you help review this again?

@liufuyang
Copy link
Contributor

liufuyang commented Feb 21, 2023

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

Successfully merging this pull request may close these issues.

8 participants