Grpc instrumentation attributes fix #3089

andrewzenkov · 2022-07-13T14:00:15Z

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

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.

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/clientUtils.ts

Line 131 in 747c404

[AttributeNames.GRPC_METHOD]: original.path,

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.

opentelemetry-js/experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/clientUtils.ts

Line 131 in 747c404

[AttributeNames.GRPC_METHOD]: original.path,

Before:
Client/server

After:
Client/Server

linux-foundation-easycla · 2022-07-13T14:00:22Z

✅ login: andrewzenkov (1aeb31e, b35929a, fbbe6f4, 4b461d6, 9d708eb)
❌ The commit (a0e6915, 3ae0bfb, 95ce965). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

weyert · 2022-07-13T22:56:57Z

experimental/packages/opentelemetry-instrumentation-grpc/src/enums/AttributeNames.ts

@@ -19,7 +19,9 @@
 */
 export enum AttributeNames {
  GRPC_KIND = 'grpc.kind', // SERVER or CLIENT
-  GRPC_METHOD = 'grpc.method',
+  GRPC_METHOD = 'rpc.method',


I think the RPC_METHOD of @opentelemetry/semantic-convention could be used for this?

See:

opentelemetry-js/packages/opentelemetry-semantic-conventions/src/trace/SemanticAttributes.ts

Line 676 in 747c404

RPC_METHOD: 'rpc.method',

nice point, will update

@weyert, resolved
There were some issues because of Typescript
It forced me to make enum as Readonly constant

weyert

LGTM

legendecas

Thank you for your contribution!

experimental/packages/opentelemetry-instrumentation-grpc/src/enums/AttributeNames.ts

experimental/packages/opentelemetry-instrumentation-grpc/src/grpc/index.ts

experimental/packages/opentelemetry-instrumentation-grpc/src/grpc-js/index.ts

andrewzenkov · 2022-07-21T16:16:50Z

@legendecas
Please check it out

legendecas

Thank you for your contribution!

legendecas · 2022-07-22T02:36:19Z

experimental/packages/opentelemetry-instrumentation-grpc/src/enums/AttributeNames.ts

+
+export const AttributeNames: Readonly<AttributesType> = {
+  RPC_SYSTEM: RPC_SYSTEM,
+  GRPC_METHOD: RPC_METHOD,


Nit: might be worth preventing introducing new names for existing semantic attributes.

Suggested change

GRPC_METHOD: RPC_METHOD,

RPC_METHOD,

might be worth directy using SemanticAttributes.RPC_METHOD directly and removing it them from AttributeNames IMO

legendecas · 2022-07-22T03:06:11Z

CLA check is still not green: #3089 (comment). You may need to revisit your commit author to ensure all your commits were authorized.

experimental/packages/opentelemetry-instrumentation-grpc/src/utils.ts

vmarchaud · 2022-07-23T12:07:51Z

experimental/packages/opentelemetry-instrumentation-grpc/src/enums/AttributeNames.ts

+
+export const AttributeNames: Readonly<AttributesType> = {
+  RPC_SYSTEM: RPC_SYSTEM,
+  GRPC_METHOD: RPC_METHOD,


might be worth directy using SemanticAttributes.RPC_METHOD directly and removing it them from AttributeNames IMO

codecov · 2022-07-23T12:10:30Z

Codecov Report

Merging #3089 (a334484) into main (df58fac) will decrease coverage by 0.40%.
The diff coverage is 100.00%.

❗ Current head a334484 differs from pull request most recent head 9d708eb. Consider uploading reports for the commit 9d708eb to get more accurate results

@@            Coverage Diff             @@
##             main    #3089      +/-   ##
==========================================
- Coverage   93.07%   92.67%   -0.41%     
==========================================
  Files         195      174      -21     
  Lines        6384     5513     -871     
  Branches     1347     1165     -182     
==========================================
- Hits         5942     5109     -833     
+ Misses        442      404      -38

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%)`	⬆️
packages/opentelemetry-resources/karma.worker.js	`0.00% <0.00%> (-100.00%)`	⬇️
...kages/opentelemetry-sdk-trace-base/karma.worker.js	`0.00% <0.00%> (-100.00%)`	⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts	`53.33% <0.00%> (-46.67%)`	⬇️
... and 36 more

…ewzenkov/opentelemetry-js into grpc-instrumentation-attributes-fix

andrewzenkov · 2022-07-25T11:40:32Z

@legendecas

I can see they are not authorized
But seems like I cannot resign them without rebase
What do you think I have to do?
Is there point to create new pr based on that having clean git history?

dyladan · 2022-07-25T20:12:10Z

Looks like some of the commits have a different author email which is making them miss out on the CLA. A rebase is probably fine since the PR squashes anyway so individual commits aren't that important.

legendecas · 2022-07-27T02:45:17Z

@andrewzenkov I would squash the commits and reset the author email of the commits to the one correctly authorized. I'm not sure about the policy regarding merging unauthorized commits. I'd be happy to defer to @dyladan's opinion.

dyladan · 2022-07-27T12:11:05Z

The policy is that no unauthorized commits will be merged

andrewzenkov · 2022-07-29T13:57:46Z

@legendecas

I created new PR as extended from it with authorized commit and all necessary changes
#3127

Please add necessary labels, and mark current PR as closed or draft :)
I decided to do it, to speed up contribution and not create a mess in all stuff

Andrei Zenkou added 2 commits July 12, 2022 16:20

Fixed missed attributes. Updated method value for some cases.

a0e6915

Updated naming of method attribute to be consistent

3ae0bfb

andrewzenkov requested a review from a team July 13, 2022 14:00

weyert suggested changes Jul 13, 2022

View reviewed changes

Added conventions from OTel package for some attributes.

95ce965

weyert approved these changes Jul 15, 2022

View reviewed changes

legendecas reviewed Jul 20, 2022

View reviewed changes

dyladan added spec-noncompliant An existing feature incorrectly or incompletely implements the OTel spec. May or may not be a bug priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect labels Jul 20, 2022

Remove useless GRPC_KIND attribute. Moved static value to variable

1aeb31e

legendecas approved these changes Jul 22, 2022

View reviewed changes

vmarchaud reviewed Jul 23, 2022

View reviewed changes

andrewzenkov and others added 3 commits July 25, 2022 12:45

Merge branch 'main' into grpc-instrumentation-attributes-fix

b35929a

Updated attributes name by using npm OTel convention directly

fbbe6f4

Merge branch 'grpc-instrumentation-attributes-fix' of github.com:andr…

4b461d6

…ewzenkov/opentelemetry-js into grpc-instrumentation-attributes-fix

Merge branch 'main' into grpc-instrumentation-attributes-fix

9d708eb

andrewzenkov mentioned this pull request Jul 29, 2022

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

Merged

2 tasks

dyladan closed this Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grpc instrumentation attributes fix #3089

Grpc instrumentation attributes fix #3089

andrewzenkov commented Jul 13, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 13, 2022 •

edited

Loading

weyert Jul 13, 2022

weyert Jul 13, 2022

andrewzenkov Jul 14, 2022

andrewzenkov Jul 15, 2022

weyert left a comment

legendecas left a comment

andrewzenkov commented Jul 21, 2022

legendecas left a comment

legendecas Jul 22, 2022

vmarchaud Jul 23, 2022

legendecas commented Jul 22, 2022

vmarchaud Jul 23, 2022

codecov bot commented Jul 23, 2022 •

edited

Loading

andrewzenkov commented Jul 25, 2022 •

edited

Loading

dyladan commented Jul 25, 2022

legendecas commented Jul 27, 2022

dyladan commented Jul 27, 2022

andrewzenkov commented Jul 29, 2022

Grpc instrumentation attributes fix #3089

Grpc instrumentation attributes fix #3089

Conversation

andrewzenkov commented Jul 13, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

Type of change

How Has This Been Tested?

linux-foundation-easycla bot commented Jul 13, 2022 • edited Loading

weyert Jul 13, 2022

Choose a reason for hiding this comment

weyert Jul 13, 2022

Choose a reason for hiding this comment

andrewzenkov Jul 14, 2022

Choose a reason for hiding this comment

andrewzenkov Jul 15, 2022

Choose a reason for hiding this comment

weyert left a comment

Choose a reason for hiding this comment

legendecas left a comment

Choose a reason for hiding this comment

andrewzenkov commented Jul 21, 2022

legendecas left a comment

Choose a reason for hiding this comment

legendecas Jul 22, 2022

Choose a reason for hiding this comment

vmarchaud Jul 23, 2022

Choose a reason for hiding this comment

legendecas commented Jul 22, 2022

vmarchaud Jul 23, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2022 • edited Loading

Codecov Report

andrewzenkov commented Jul 25, 2022 • edited Loading

dyladan commented Jul 25, 2022

legendecas commented Jul 27, 2022

dyladan commented Jul 27, 2022

andrewzenkov commented Jul 29, 2022

andrewzenkov commented Jul 13, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jul 13, 2022 •

edited

Loading

codecov bot commented Jul 23, 2022 •

edited

Loading

andrewzenkov commented Jul 25, 2022 •

edited

Loading