opentelemetry-js
opentelemetry-js copied to clipboard
fix(instrumentation-grpc): always set grpc semcov status code attribute with numeric value
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.
rpc.grpc.status_codeattribute was populated with string values which is not spec compliant. now it is filled with numbers #3054- in some places, instrumentation used the
SpanStatusenum to populate the attribute, instead of the gRPC status codes, which was incorrect. #2984 - the
rpc.grpc.status_codewas not populated in some cases. Now it is always filled where known. - the attribute value was not asserted at all in any unittests. this PR adds the relevant assert to verify the attribute value.
- grpc status code OK (===0) was read from
moduleExportswhich made the code complicated and required to pass around the module exports just to read a constant value0. 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
Codecov Report
Merging #3076 (1ffd797) into main (db0ecc3) will decrease coverage by
0.36%. The diff coverage isn/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
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.
Sorry for the delay, rebased and fixed the conflicts