Skip to content

Consolidate HTTP span data conventions with OpenTelemetry#2093

Merged
sl0thentr0py merged 1 commit intomasterfrom
neel/starfish-http
Sep 4, 2023
Merged

Consolidate HTTP span data conventions with OpenTelemetry#2093
sl0thentr0py merged 1 commit intomasterfrom
neel/starfish-http

Conversation

@sl0thentr0py
Copy link
Copy Markdown
Member

@sl0thentr0py sl0thentr0py commented Aug 17, 2023 •
edited
Loading

in preparation for some of our new performance UIs we're consolidating span data keys with OpenTelemetry with a new Span::DataConventions module.

This PR handles the HTTP spans.

see getsentry/team-sdks#20

@sl0thentr0py sl0thentr0py requested a review from st0012 August 17, 2023 13:21
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 17, 2023 •
edited
Loading

Codecov Report

Patch coverage: 69.23% and project coverage change: -0.01% ⚠️

Comparison is base (86dfaea) 83.25% compared to head (fd17ccf) 83.25%.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2093      +/-   ##
==========================================
- Coverage   83.25%   83.25%   -0.01%     
==========================================
  Files         119      119              
  Lines        5674     5679       +5     
==========================================
+ Hits         4724     4728       +4     
- Misses        950      951       +1
Files Changed Coverage Δ
sentry-ruby/lib/sentry/net/http.rb 29.78% <0.00%> (ø)
...rails/tracing/action_controller_subscriber_spec.rb 100.00% <100.00%> (ø)
sentry-rails/spec/sentry/rails/tracing_spec.rb 99.42% <100.00%> (ø)
sentry-ruby/lib/sentry/span.rb 87.95% <100.00%> (+0.77%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

antonpirker
antonpirker approved these changes Aug 17, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@antonpirker antonpirker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

st0012
st0012 reviewed Aug 20, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we did a span name change in #1923, some users reported that it "broke" their filtering logic and resulted in higher-than-expected usages. Since status code could be a common method for filtering, I'm a bit concerned that similar issue could happen again. Should we still set status_code so users don't get breaking change in a minor version upgrade?

@st0012 st0012 added this to the 5.11.0 milestone Aug 20, 2023
szokeasaurusrex
szokeasaurusrex approved these changes Aug 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@sl0thentr0py
Copy link
Copy Markdown
Member Author

sl0thentr0py commented Aug 28, 2023

@st0012 good point, but this is data whereas those were ops, I think it's fine to clean this up without consequences. The span status is still the same and that's what I'd recommend for filtering just in case.

@sl0thentr0py sl0thentr0py merged commit 9f055b5 into master Sep 4, 2023
@sl0thentr0py sl0thentr0py deleted the neel/starfish-http branch September 4, 2023 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@szokeasaurusrex szokeasaurusrex szokeasaurusrex approved these changes

+2 more reviewers

@antonpirker antonpirker antonpirker approved these changes

@st0012 st0012 st0012 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

5.11.0

Development

Successfully merging this pull request may close these issues.

4 participants

@sl0thentr0py @antonpirker @st0012 @szokeasaurusrex