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

feat: Include unfinished spans in transactions #1592

Merged
merged 4 commits into from
Dec 29, 2021

Conversation

brustolin
Copy link
Contributor

@brustolin brustolin commented Dec 27, 2021

📜 Description

Instead of removing unfinished spans from the transaction in order to avoid the transaction to be discarded, we close all unfinished spans with deadline_exceeded status.

💡 Motivation and Context

closes #1303

💚 How did you test it?

Unit tests

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@github-actions
Copy link

github-actions bot commented Dec 27, 2021

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against de3a462

@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2021

Codecov Report

Merging #1592 (de3a462) into master (6c4db1d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1592   +/-   ##
=======================================
  Coverage   95.94%   95.95%           
=======================================
  Files         157      157           
  Lines        7061     7068    +7     
=======================================
+ Hits         6775     6782    +7     
  Misses        286      286           
Impacted Files Coverage Δ
Sources/Sentry/SentrySpan.m 100.00% <100.00%> (ø)
Sources/Sentry/SentryTracer.m 98.11% <100.00%> (+0.05%) ⬆️

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 6c4db1d...de3a462. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Good job, two comments to address.

if (finishedSpan != self.rootSpan) {
// calls finish on the rootSpan. If the root span is finished, it alread finished every
// unfinished child.
if (finishedSpan != self.rootSpan && !self.rootSpan.isFinished) {
Copy link
Member

Choose a reason for hiding this comment

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

m: I think it would be better to avoid the endless loop in canBeFinished not here. So canBeFinished should check if finishedSpan != self.rootSpan.

Tests/SentryTests/Performance/SentryTracerTests.swift Outdated Show resolved Hide resolved
Sources/Sentry/SentryTracer.m Show resolved Hide resolved
brustolin and others added 2 commits December 28, 2021 14:44
Co-authored-by: Philipp Hofmann <philipp.hofmann@sentry.io>
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Awesome thanks 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include unfinished spans in transactions
3 participants