Skip to content

Add Transaction#set_context api#1947

Merged
sl0thentr0py merged 6 commits intomasterfrom
neel/add-tx-contexts
Nov 29, 2022
Merged

Add Transaction#set_context api#1947
sl0thentr0py merged 6 commits intomasterfrom
neel/add-tx-contexts

Conversation

@sl0thentr0py
Copy link
Member

@sl0thentr0py sl0thentr0py commented Nov 23, 2022

  • 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

@sl0thentr0py sl0thentr0py mentioned this pull request Nov 23, 2022
Closed
@sl0thentr0py sl0thentr0py requested a review from st0012 November 23, 2022 15:38
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022 •
edited by codecov bot
Loading

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.34%. Comparing base (3fad30c) to head (d8a3d5e).
⚠️ Report is 466 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1947      +/-   ##
==========================================
- Coverage   98.35%   98.34%   -0.01%     
==========================================
  Files         151      151              
  Lines        9414     9450      +36     
==========================================
+ Hits         9259     9294      +35     
- Misses        155      156       +1

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

st0012
st0012 reviewed Nov 24, 2022
View reviewed changes
Copy link
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.

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?

@sl0thentr0py
Copy link
Member Author

sl0thentr0py commented Nov 28, 2022

@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.

@sl0thentr0py sl0thentr0py force-pushed the neel/otel-expose-finish branch from f2d6d71 to e863e25 Compare November 28, 2022 13:29
@st0012
Copy link
Contributor

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

sl0thentr0py commented Nov 28, 2022

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.

st0012
st0012 reviewed Nov 29, 2022
View reviewed changes
Copy link
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.

This seems to be mixed with other PRs' changes?

@sl0thentr0py sl0thentr0py force-pushed the neel/otel-expose-finish branch from e863e25 to 85d1c67 Compare November 29, 2022 12:57
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 •
edited
Loading

@st0012 it needed a merge, done now

@sl0thentr0py sl0thentr0py requested a review from st0012 November 29, 2022 13:20
@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

Reviewers

@st0012 st0012 Awaiting requested review from st0012

1 more reviewer

@AbhiPrasad AbhiPrasad AbhiPrasad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sl0thentr0py sl0thentr0py

Labels

sentry-ruby Traces

Projects

None yet

Milestone

Basic OpenTelemetry (oTel) Support

Development

Successfully merging this pull request may close these issues.

4 participants

@sl0thentr0py @codecov-commenter @st0012 @AbhiPrasad