Skip to content

Add Transaction#set_context api - #1947

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

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

Conversation

@sl0thentr0py

Copy link
Copy Markdown
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

codecov-commenter commented Nov 23, 2022

Copy link
Copy Markdown

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.

@st0012st0012 left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadsentry-ruby/lib/sentry/transaction.rb
@sl0thentr0py

Copy link
Copy Markdown
MemberAuthor

@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
Copy Markdown
Contributor

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
Copy Markdown
MemberAuthor

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.

@st0012st0012 left a comment

Copy link
Copy Markdown
Contributor

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 masterNovember 29, 2022 12:59
@sl0thentr0py

sl0thentr0py commented Nov 29, 2022

Copy link
Copy Markdown
MemberAuthor

@st0012 it needed a merge, done now

@sl0thentr0py
sl0thentr0py merged commit b3291a5 into masterNov 29, 2022
@sl0thentr0py
sl0thentr0py deleted the neel/add-tx-contexts branch November 29, 2022 13:49
Sign up for freeto 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

@sl0thentr0py@codecov-commenter@st0012@AbhiPrasad