Skip to content

fix: Reset transaction in scope if span is reset#1125

Merged
marandaneto merged 6 commits into
getsentry:mainfrom
lColinDl:fix-transaction-kept-alive
Nov 16, 2022
Merged

fix: Reset transaction in scope if span is reset#1125
marandaneto merged 6 commits into
getsentry:mainfrom
lColinDl:fix-transaction-kept-alive

Conversation

@lColinDl

@lColinDl lColinDl commented Nov 10, 2022

Copy link
Copy Markdown
Contributor

📜 Description

The private _transaction is not set anymore when setting a span on Scope. Instead the code relies on the public getter for transaction that already had logic in using the span name as the transaction if available. By doing so the transaction name is set back to null after a transaction finishes.

💡 Motivation and Context

Previously events had a transaction string set of transactions that already finished earlier. Because after a transaction finished, the _transaction field in Scopewas never reset.

💚 How did you test it?

Added new new tests. Existing tests still work.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated the docs if needed
  • All tests passing
  • No breaking changes

🔮 Next steps

@marandaneto

marandaneto commented Nov 11, 2022

Copy link
Copy Markdown
Contributor

@lColinDl I don't really follow the issue, the transaction name is bound to the span object, so it is doing what it should do.
If there's a span bound to the scope, return the transaction name otherwise not.
There should not be a transaction name if there's no span in the scope.

@ueman

ueman commented Nov 11, 2022

Copy link
Copy Markdown
Collaborator

There should not be a transaction name if there's no span in the scope.

That's exactly what this PR does, because that's currently not the case.

When a transaction finishes, the transaction name is still added to events from after the transaction.

@marandaneto

marandaneto commented Nov 11, 2022

Copy link
Copy Markdown
Contributor

There should not be a transaction name if there's no span in the scope.

That's exactly what this PR does, because that's currently not the case.

When a transaction finishes, the transaction name is still added to events from after the transaction.

That's right, but then this code is breaking the transaction feature anyway.
Maybe we should clean the transaction name from the scope here ->

sentry-dart/dart/lib/src/sentry_tracer.dart

Lines 113 to 118 in 25e9b59

// remove from scope
await _hub.configureScope((scope) {
if (scope.span == this) {
scope.span = null;
}
});

scope.transaction (String) can be used standalone without the APM feature (spans), the difference is that when using the APM feature, we fallback to the span name instead of using the transaction String, and both should work as well, the current PR breaks the other feature, or I am missing something.

@lColinDl

lColinDl commented Nov 14, 2022

Copy link
Copy Markdown
Contributor Author

the current PR breaks the other feature, or I am missing something.

I don't see how this PR is breaking existing desired behavior. I intentionally tried to stay as close to the original behavior as possible. Setting the transaction name separately should still work as before. Notice also how all existing tests stayed untouched in their behavior.

Unfortunately I can't quite follow the rest of your argument. If you have a better solution to fixing the problem, I obviously don't mind it.

@marandaneto

marandaneto commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

the current PR breaks the other feature, or I am missing something.

I don't see how this PR is breaking existing desired behavior. I intentionally tried to stay as close to the original behavior as possible. Setting the transaction name separately should still work as before. Notice also how all existing tests stayed untouched in their behavior.

Unfortunately I can't quite follow the rest of your argument. If you have a better solution to fixing the problem, I obviously don't mind it.

You're right, sorry for the confusion, I was misreading the spec.
Can you add a changelog entry? so we can merge it :)
Thanks for doing this.

@lColinDl

lColinDl commented Nov 15, 2022

Copy link
Copy Markdown
Contributor Author

Can you add a changelog entry? so we can merge it :)

Done: 8e21c1c

@marandaneto
marandaneto enabled auto-merge (squash) November 15, 2022 12:08
@codecov-commenter

codecov-commenter commented Nov 15, 2022 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1125      +/-   ##
==========================================
- Coverage   90.09%   90.08%   -0.01%     
==========================================
  Files         117      117              
  Lines        3605     3602       -3     
==========================================
- Hits         3248     3245       -3     
  Misses        357      357
Impacted Files Coverage Δ
dart/lib/src/scope.dart 96.77% <100.00%> (-0.06%) ⬇️

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.

marandaneto
marandaneto reviewed Nov 15, 2022
View reviewed changes
Comment thread dart/lib/src/scope.dart
auto-merge was automatically disabled November 15, 2022 12:24

Base branch was modified

@marandaneto
marandaneto enabled auto-merge (squash) November 16, 2022 19:46
@marandaneto
marandaneto merged commit 379d7a8 into getsentry:main Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brustolin brustolin Awaiting requested review from brustolin
@krystofwoldrich krystofwoldrich Awaiting requested review from krystofwoldrich
1 more reviewer
@marandaneto marandaneto marandaneto approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lColinDl @marandaneto @ueman @codecov-commenter