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

feat: add forming grpc reqeuest url if rpc attributes are present #280

Merged
merged 4 commits into from
Nov 8, 2021

Conversation

kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Nov 2, 2021

Description

For RPC methods, we show URL/URI as a combination of rpc.service and rpc.method. The same information is also available as Span Name - https://github.com/open-telemetry/opentelemetry-specification/blob/3e380e249f60c3a5f68746f5e84d10195ba41a79/specification/trace/semantic_conventions/rpc.md#span-name

It has been observed that spanName is not correctly set on certain scenarios. However, the span contains rpc.service and rpc.method attribute correctly set. So, as part of this PR, we will first look into RPC attributes if a protocol is grpc, and if those are not present, we will fall back to eventName which is nothing but spanName.

Testing

Added unit tests

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #280 (5d8d97a) into main (ece2263) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main     #280   +/-   ##
=========================================
  Coverage     79.07%   79.07%           
  Complexity     1231     1231           
=========================================
  Files           110      110           
  Lines          4861     4861           
  Branches        440      440           
=========================================
  Hits           3844     3844           
  Misses          813      813           
  Partials        204      204           
Flag Coverage Δ
unit 79.07% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ewgenerator/generators/SpanEventViewGenerator.java 92.12% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ece2263...5d8d97a. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit 30f0a7b into main Nov 8, 2021
@kotharironak kotharironak deleted the fix-grpc-request-url branch November 8, 2021 11:49
@github-actions
Copy link

github-actions bot commented Nov 8, 2021

Unit Test Results

  73 files  ±0    73 suites  ±0   1m 1s ⏱️ -6s
390 tests +3  390 ✔️ +3  0 💤 ±0  0 ❌ ±0 

Results for commit 30f0a7b. ± Comparison against base commit ece2263.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants