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

Add Transaction#set_context api #1947

Merged
merged 6 commits into from
Nov 29, 2022
Merged

Add Transaction#set_context api #1947

merged 6 commits into from
Nov 29, 2022

Conversation

sl0thentr0py
Copy link
Member

  • allow arbitrary contexts to be set on the transaction directly
  • also including two other changes which are not changelog worthy
    • add internal Transaction#set_name api to set name on transaction directly
    • extract sentry-trace regex logic into Transaction.extract_sentry_trace helper for reusability

@codecov-commenter
Copy link

Codecov Report

Base: 98.35% // Head: 98.35% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (7fedc8e) compared to base (f2d6d71).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@                     Coverage Diff                     @@
##           neel/otel-expose-finish    #1947      +/-   ##
===========================================================
- Coverage                    98.35%   98.35%   -0.01%     
===========================================================
  Files                          151      151              
  Lines                         9380     9404      +24     
===========================================================
+ Hits                          9226     9249      +23     
- Misses                         154      155       +1     
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/transaction.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction_event.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/client_spec.rb 96.70% <100.00%> (+0.04%) ⬆️
sentry-ruby/spec/sentry/transaction_spec.rb 100.00% <100.00%> (ø)
sentry-resque/lib/sentry/resque.rb 97.22% <0.00%> (-2.78%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

This also feel like an independent change to me so can base on master directly.
Can we add test case to clarify the precedence of Scope and Transaction's contexts, likely in sentry_spec.rb or hub_spec.rb?

sentry-ruby/lib/sentry/transaction.rb Show resolved Hide resolved
@sl0thentr0py
Copy link
Member Author

@st0012 regarding the based on master, I'll merge them in order so they'll each get their updated base as master.
I'll have to solve changelog merge conflicts every time otherwise.

@st0012
Copy link
Collaborator

st0012 commented Nov 28, 2022

IMO, all of these PRs deserve their own commits on master and independent changlog entries (not under the telemetry one) because 1) they're self-contained and 2) they also affect people who don't use sentry-opentelemetry.
I understand it's a bit tedious to resolve conflicts over and over, but it's worth doing to keep the history clear.

@sl0thentr0py
Copy link
Member Author

I will move the changelog entries, but the history will be clear, I don't want to re-create all these branches.
If they get merged in order, the base will be master every time, they are just chained right now.

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.

This seems to be mixed with other PRs' changes?

Base automatically changed from neel/otel-expose-finish to master November 29, 2022 12:59
@sl0thentr0py
Copy link
Member Author

sl0thentr0py commented Nov 29, 2022

@st0012 it needed a merge, done now

@sl0thentr0py sl0thentr0py merged commit b3291a5 into master Nov 29, 2022
@sl0thentr0py sl0thentr0py deleted the neel/add-tx-contexts branch November 29, 2022 13:49
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.

4 participants