opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

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

Open blumamir opened this issue 3 years ago • 2 comments
trafficstars

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 #3054
  2. in some places, instrumentation used the SpanStatus enum to populate the attribute, instead of the gRPC status codes, which was incorrect. #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.

  • [x] 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:

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

blumamir avatar Jul 04 '22 16:07 blumamir

Codecov Report

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

:exclamation: 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

codecov[bot] avatar Jul 04 '22 16:07 codecov[bot]

The Lint fail in CI seems unrelated to this PR:

> [email protected] 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.

blumamir avatar Jul 04 '22 17:07 blumamir

Sorry for the delay, rebased and fixed the conflicts

blumamir avatar Nov 30 '22 08:11 blumamir