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

improve readability of traces #190

Closed
wants to merge 1 commit into from

Conversation

iosifch
Copy link

@iosifch iosifch commented Sep 4, 2023

Hi! I'm back with the proposed changes! As I said in the previous pull request, the changes should aim to a better readability of traces.

previous pull request

@iosifch iosifch requested a review from a team September 4, 2023 11:22
@welcome
Copy link

welcome bot commented Sep 4, 2023

Thanks for opening your first pull request! If you haven't yet signed our Contributor License Agreement (CLA), then please do so that we can accept your contribution. A link should appear shortly in this PR if you have not already signed one.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 4, 2023

CLA Missing ID CLA Not Signed

@iosifch iosifch force-pushed the client-server-request branch from 50b3859 to 638c33f Compare September 4, 2023 11:26
@codecov
Copy link

codecov bot commented Sep 4, 2023

Codecov Report

Merging #190 (fe6f862) into main (c8e79a8) will increase coverage by 2.81%.
The diff coverage is n/a.

❗ Current head fe6f862 differs from pull request most recent head 06e35ef. Consider uploading reports for the commit 06e35ef to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #190      +/-   ##
============================================
+ Coverage     28.60%   31.41%   +2.81%     
+ Complexity      472      443      -29     
============================================
  Files            47       43       -4     
  Lines          1755     1598     -157     
============================================
  Hits            502      502              
+ Misses         1253     1096     -157     
Flag Coverage Δ
7.4 52.29% <ø> (ø)
8.0 31.09% <ø> (+3.23%) ⬆️
8.1 31.16% <ø> (+3.23%) ⬆️
8.2 31.43% <ø> (+2.81%) ⬆️

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


Continue to review full report in Codecov by Sentry.

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

@iosifch iosifch force-pushed the client-server-request branch from 638c33f to 06e35ef Compare September 4, 2023 11:48
@iosifch
Copy link
Author

iosifch commented Sep 4, 2023

/cc @beniamin @brettmc

@brettmc
Copy link
Collaborator

brettmc commented Sep 6, 2023

We discussed this at SIG just now. Somebody pointed out this part of the spec which I couldn't find earlier.

It's clear about how http server spans should be named, and also client spans. We're not quite getting either of them right at the moment, it seems :(

server spans: {method} {optional route name}
client spans: {method} (and MUST NOT use the URI).

Based on that this PR doesn't meet the spec requirements. However, we'd love it if this PR or a new one could bring http span names into alignment with the spec.

@iosifch
Copy link
Author

iosifch commented Sep 6, 2023

From what you say I understand that the only issue with the pull request right now is that it adds the hostname for client request spans.

@brettmc
Copy link
Collaborator

brettmc commented Sep 6, 2023

From what you say I understand that the only issue with the pull request right now is that it adds the hostname for client request spans.

I think also that it adds "unknown route", which the spec says to leave out if not known.

@iosifch iosifch closed this Sep 26, 2023
@iosifch iosifch deleted the client-server-request branch September 26, 2023 19:54
@brettmc brettmc mentioned this pull request Feb 28, 2024
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