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

GRPC instrumentation. Added new attributes according to the OpenTelemetry rpc convention #3127

Merged

Conversation

andrewzenkov
Copy link
Contributor

This PR is extending old one. Feel free to check it if needs comments

Which problem is this PR solving?

Opentelemtry grpc instrumentation is not consistent in terms of creating grpc attributes in requests.
For example in Java/Python instrumentations it contains necessary attributes for grpc connection:

  • rpc.method
  • rpc.service
  • rpc.system

image

But these attributes are missed in js grpc instrumentation. It doesn't set attributes.
Additionally, if compare Python/Java and Nodejs attributes.
Already existing attribute rpc.method is implemented incorrectly in Nodejs.

Fixes # (issue)

Short description of the changes

  • Renamed rpc.method from grpc.method to rpc.method attribute (Client, Server)
  • Added rpc.service attribute (Client, Server)
  • Added rpc.system attribute (Client, Server)
  • Fixedrpc.method value calculating for some use case. (it should be method, but value is just full path)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Create GRPC example, run opentelemetry SDK using authoinstrumentation, or manually set new GrpcInstrumentation(), you will see that Grpc instrumentation detects grpc connection and creates spans but with missing attributes:

  • rpc.method (server?/client?)
  • rpc.system (server/client)
  • rpc.service (server/client)

And for client rpc.method has incorrect value.

Before:
Client/server
image

After:
Client/Server
image

@andrewzenkov andrewzenkov requested a review from a team July 29, 2022 13:55
@dyladan dyladan added priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug labels Jul 29, 2022
@dyladan
Copy link
Member

dyladan commented Jul 29, 2022

Title isn't very specific as to what is actually happening. Please make a more descriptive title (and add it to changelog)

@codecov
Copy link

codecov bot commented Jul 29, 2022

Codecov Report

Merging #3127 (3657cd2) into main (fc4dcba) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3127      +/-   ##
==========================================
- Coverage   93.10%   93.09%   -0.01%     
==========================================
  Files         195      196       +1     
  Lines        6384     6391       +7     
  Branches     1348     1348              
==========================================
+ Hits         5944     5950       +6     
- Misses        440      441       +1     
Impacted Files Coverage Δ
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts 92.50% <ø> (-0.10%) ⬇️
...metry-instrumentation-grpc/src/grpc/clientUtils.ts 88.23% <ø> (-0.18%) ⬇️
...y-instrumentation-grpc/src/enums/AttributeNames.ts 100.00% <100.00%> (ø)
...-instrumentation-grpc/src/enums/AttributeValues.ts 100.00% <100.00%> (ø)
...elemetry-instrumentation-grpc/src/grpc-js/index.ts 91.26% <100.00%> (+0.35%) ⬆️
...entelemetry-instrumentation-grpc/src/grpc/index.ts 93.27% <100.00%> (+0.17%) ⬆️
...es/opentelemetry-instrumentation-grpc/src/utils.ts 94.73% <100.00%> (+0.79%) ⬆️
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

@andrewzenkov andrewzenkov changed the title Grpc instrumentation attributes fix GRPC instrumentation. Added new attributes according to the OpenTelemetry rpc convention Jul 29, 2022
@vmarchaud vmarchaud merged commit e76cb8b into open-telemetry:main Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants