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

Consolidate HTTP span data conventions with OpenTelemetry #2093

Merged
merged 1 commit into from Sep 4, 2023

Conversation

sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Aug 17, 2023

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

@codecov
Copy link

codecov bot commented Aug 17, 2023

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.

Copy link
Member

@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

Copy link
Collaborator

@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
Copy link
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
Member Author

@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
96 of 98 checks passed
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants