Skip to content

Let Transaction constructor take an optional hub argument#1384

Merged
st0012 merged 2 commits intomasterfrom
match-transaction-implementation
Apr 10, 2021
Merged

Let Transaction constructor take an optional hub argument#1384
st0012 merged 2 commits intomasterfrom
match-transaction-implementation

Conversation

@st0012
Copy link
Copy Markdown
Contributor

@st0012 st0012 commented Apr 5, 2021

  1. This makes the SDK matches sentry-python's implementation.
  2. Having a hub inside a Span/Transaction may be necessary for future features.
  3. This eliminates the optional configuration arguments in different places, which makes it easier to understand and refactor.

@st0012 st0012 added this to the sentry-ruby-4.3.2 milestone Apr 5, 2021
@st0012 st0012 self-assigned this Apr 5, 2021
@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 5, 2021 •
edited
Loading

Codecov Report

Merging #1384 (b73f586) into master (f1bbcf2) will decrease coverage by 3.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1384      +/-   ##
==========================================
- Coverage   98.09%   95.00%   -3.10%     
==========================================
  Files         208       98     -110     
  Lines        9223     4681    -4542     
==========================================
- Hits         9047     4447    -4600     
- Misses        176      234      +58
Impacted Files Coverage Δ
sentry-ruby/lib/sentry/hub.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/span.rb 100.00% <100.00%> (ø)
sentry-ruby/lib/sentry/transaction.rb 100.00% <100.00%> (ø)
sentry-ruby/spec/sentry/transaction_spec.rb 100.00% <100.00%> (ø)
.../lib/sentry/rails/rescued_exception_interceptor.rb 27.77% <0.00%> (-72.23%) ⬇️
sentry-rails/lib/sentry/rails/railtie.rb 29.82% <0.00%> (-70.18%) ⬇️
...ntry/rails/tracing/action_controller_subscriber.rb 37.50% <0.00%> (-62.50%) ⬇️
...ls/lib/sentry/rails/tracing/abstract_subscriber.rb 34.61% <0.00%> (-61.54%) ⬇️
sentry-rails/app/jobs/sentry/send_event_job.rb 16.66% <0.00%> (-61.12%) ⬇️
...entry-rails/lib/sentry/rails/capture_exceptions.rb 39.13% <0.00%> (-60.87%) ⬇️
... and 119 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f1bbcf2...b73f586. Read the comment docs.

@st0012 st0012 requested a review from jan-auer April 5, 2021 13:32
@st0012 st0012 mentioned this pull request Apr 5, 2021
Merged
@st0012 st0012 modified the milestones: sentry-ruby-4.3.2, 4.4.0 Apr 5, 2021
@st0012 st0012 force-pushed the match-transaction-implementation branch from a5c9950 to 9d4345c Compare April 5, 2021 14:05
@rhcarvalho rhcarvalho requested review from a team, kamilogorek and rhcarvalho and removed request for a team and jan-auer April 6, 2021 07:38
rhcarvalho
rhcarvalho suggested changes Apr 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Good change 👍
I have one suggestion to simplify the relationship between transactions and hub (and spans should not know about hub).

@st0012 st0012 force-pushed the match-transaction-implementation branch 2 times, most recently from 29d3432 to 1ed7c71 Compare April 7, 2021 06:48
@st0012 st0012 changed the title Let Span constructor take an optional hub argument Let Transaction constructor take an optional hub argument Apr 7, 2021
@st0012 st0012 force-pushed the match-transaction-implementation branch from 1ed7c71 to d0f76d7 Compare April 7, 2021 06:58
@st0012 st0012 requested a review from rhcarvalho April 8, 2021 03:33
rhcarvalho
rhcarvalho approved these changes Apr 8, 2021
View reviewed changes
sentry-ruby/lib/sentry/transaction.rb
@@ -108,6 +111,8 @@ def set_initial_sample_decision(sampling_context:, configuration: Sentry.configu
end

def finish(hub: nil)
Copy link
Copy Markdown
Contributor

@rhcarvalho rhcarvalho Apr 8, 2021

Choose a reason for hiding this comment

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

I think callers should never pass a different hub here than the one used to start the transaction, but I'm okay if it is not removed from the API (I'm aware Python has it for historical reasons, committed 2 years ago...)

st0012 added 2 commits April 9, 2021 17:44
@st0012
Let Transaction constructor take an optional hub argument
7483396 
1. This makes the SDK matches sentry-python's implementation.
2. Having a hub inside a Transaction may be necessary for future
   features.
3. This eliminates the optional configuration arguments in different
   places, which makes it easier to understand and refactor.
@st0012
Update changelog
259b329
@st0012 st0012 force-pushed the match-transaction-implementation branch from d0f76d7 to 259b329 Compare April 9, 2021 09:44
@st0012 st0012 merged commit 2ccfcf3 into master Apr 10, 2021
@st0012 st0012 deleted the match-transaction-implementation branch April 10, 2021 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kamilogorek kamilogorek Awaiting requested review from kamilogorek

1 more reviewer

@rhcarvalho rhcarvalho rhcarvalho approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@st0012 st0012

Labels

Refactoring sentry-ruby Traces

Projects

None yet

Milestone

4.4.0

Development

Successfully merging this pull request may close these issues.

3 participants

@st0012 @codecov-io @rhcarvalho