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.

Sign up for GitHub

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

Jump to bottom

fix(instrumentation-grpc): always set grpc semcov status code attribute with numeric value #3076

Merged
merged 12 commits into from
Nov 30, 2022
Merged

fix(instrumentation-grpc): always set grpc semcov status code attribute with numeric value #3076

merged 12 commits into from
Nov 30, 2022

Conversation

blumamir
Copy link
Member

@blumamir blumamir commented Jul 4, 2022

Which problem is this PR solving?

fixes #2984
fixes #3052
extends #2988 (which is not finalized: fixes an issue in one place out of 3 and no tests. no comment from the author for over a month).

Short description of the changes

This PR fixes various issues with grpc instrumentation in the status code attribute.

  1. rpc.grpc.status_code attribute was populated with string values which is not spec compliant. now it is filled with numbers fix(instrumentation-http): add http.host attribute before sending the request #3054
  2. in some places, instrumentation used the SpanStatus enum to populate the attribute, instead of the gRPC status codes, which was incorrect. Success status code for grpc (rpc.grpc.status_code) incorrect using grpc-js #2984
  3. the rpc.grpc.status_code was not populated in some cases. Now it is always filled where known.
  4. the attribute value was not asserted at all in any unittests. this PR adds the relevant assert to verify the attribute value.
  5. grpc status code OK (===0) was read from moduleExports which made the code complicated and required to pass around the module exports just to read a constant value 0. This pattern is removed in favor of instrumentation constant. Read more discussion here

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)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

I added the relevant assert to unit tests. This revealed other places where the status code was not populated which I was not aware of and I also fixed them in this PR.

I am not deeply familiar with gRPC and this instrumentation, so please review it thoroughly :)

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@blumamir blumamir requested a review from a team July 4, 2022 16:28
@codecov
Copy link

codecov bot commented Jul 4, 2022 •
edited
Loading

Codecov Report

Merging #3076 (1ffd797) into main (db0ecc3) will decrease coverage by 0.36%.
The diff coverage is n/a.

❗ Current head 1ffd797 differs from pull request most recent head 3bbbbc8. Consider uploading reports for the commit 3bbbbc8 to get more accurate results

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3076      +/-   ##
==========================================
- Coverage   93.31%   92.94%   -0.37%     
==========================================
  Files         247      219      -28     
  Lines        7387     5218    -2169     
  Branches     1518     1041     -477     
==========================================
- Hits         6893     4850    -2043     
+ Misses        494      368     -126
Impacted Files Coverage Δ
...ry-instrumentation-grpc/src/grpc-js/clientUtils.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
...metry-instrumentation-grpc/src/grpc/clientUtils.ts
...entelemetry-instrumentation-grpc/src/grpc/index.ts
...metry-instrumentation-grpc/src/grpc/serverUtils.ts
...ackages/opentelemetry-shim-opentracing/src/shim.ts
...es/opentelemetry-instrumentation-grpc/src/utils.ts
...elemetry-instrumentation-grpc/src/grpc-js/index.ts
...ckages/opentelemetry-browser-detector/src/types.ts
.../exporter-trace-otlp-grpc/src/OTLPTraceExporter.ts
... and 18 more

@blumamir blumamir mentioned this pull request Jul 4, 2022
Closed
4 tasks
@blumamir
Copy link
Member Author

blumamir commented Jul 4, 2022

The Lint fail in CI seems unrelated to this PR:

> opentelemetry@0.1.0 docs:test /home/runner/work/opentelemetry-js/opentelemetry-js
> linkinator docs --silent --retry && linkinator doc/*.md --silent --retry
🏊‍♂️ crawling docs
[525] https://img.shields.io/codecov/c/github/open-telemetry/opentelemetry-js?style=for-the-badge
docs
  [525] https://img.shields.io/codecov/c/github/open-telemetry/opentelemetry-js?style=for-the-badge
ERROR: Detected 1 broken links. Scanned 101 links in 1.909 seconds.

dyladan
dyladan approved these changes Jul 5, 2022
View reviewed changes
experimental/CHANGELOG.md Outdated Show resolved Hide resolved
@blumamir blumamir changed the title fix(instrumentation-grpc): always set status code attribute with numeric value fix(instrumentation-grpc): always set grpc semcov status code attribute with numeric value Jul 5, 2022
@blumamir
Copy link
Member Author

Sorry for the delay, rebased and fixed the conflicts

@blumamir blumamir merged commit b152497 into open-telemetry:main Nov 30, 2022
@dyladan dyladan mentioned this pull request Dec 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pichlermarc pichlermarc pichlermarc approved these changes

@legendecas legendecas legendecas approved these changes

@dyladan dyladan dyladan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@blumamir @dyladan @legendecas @pichlermarc