Skip to content

fix: Return SentryNoOpSpan when starting a child on a finished transaction #2239

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

Merged
merged 13 commits into from
Oct 3, 2022

Conversation

kevinrenskers
Copy link
Contributor

📜 Description

Return a SentryNoOpSpan when starting a child on a finished transaction, and log a warning.

💡 Motivation and Context

Closes #1624.

💚 How did you test it?

Unit test.

📝 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

Sorry, something went wrong.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 29, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1243.76 ms 1262.20 ms 18.44 ms
Size 20.51 KiB 333.10 KiB 312.60 KiB

Baseline results on branch: master

Startup times

Revision Plain With Sentry Diff
0fdf0b2 1243.92 ms 1250.86 ms 6.94 ms
b172a8b 1257.68 ms 1272.38 ms 14.70 ms
075911d 1256.73 ms 1271.22 ms 14.49 ms
0fdf0b2 1266.27 ms 1277.90 ms 11.63 ms
a5ca27b 1231.31 ms 1252.56 ms 21.25 ms
864c39a 1239.45 ms 1256.76 ms 17.31 ms
6177f2d 1206.55 ms 1226.20 ms 19.65 ms
4a66f00 1259.84 ms 1281.66 ms 21.82 ms
4c1fab4 1222.96 ms 1251.00 ms 28.04 ms
864c39a 1191.14 ms 1233.38 ms 42.24 ms

App size

Revision Plain With Sentry Diff
0fdf0b2 20.51 KiB 332.90 KiB 312.39 KiB
b172a8b 20.51 KiB 331.79 KiB 311.28 KiB
075911d 20.51 KiB 332.90 KiB 312.39 KiB
0fdf0b2 20.51 KiB 332.90 KiB 312.39 KiB
a5ca27b 20.51 KiB 331.81 KiB 311.31 KiB
864c39a 20.51 KiB 335.57 KiB 315.06 KiB
6177f2d 20.51 KiB 332.90 KiB 312.40 KiB
4a66f00 20.51 KiB 331.79 KiB 311.28 KiB
4c1fab4 20.51 KiB 331.79 KiB 311.28 KiB
864c39a 20.51 KiB 335.57 KiB 315.06 KiB

Previous results on branch: fix/1624-finished-span-start-child

Startup times

Revision Plain With Sentry Diff
320c2dd 1227.33 ms 1246.36 ms 19.03 ms
766f1f3 1231.44 ms 1247.42 ms 15.98 ms
7202f4f 1268.58 ms 1286.20 ms 17.62 ms
23978c6 1232.63 ms 1254.46 ms 21.83 ms
8468b08 1260.12 ms 1272.47 ms 12.35 ms
e9ec817 1235.72 ms 1258.86 ms 23.14 ms
137e8fe 1235.31 ms 1253.34 ms 18.03 ms

App size

Revision Plain With Sentry Diff
320c2dd 20.51 KiB 333.10 KiB 312.60 KiB
766f1f3 20.51 KiB 333.10 KiB 312.60 KiB
7202f4f 20.51 KiB 332.69 KiB 312.18 KiB
23978c6 20.51 KiB 333.03 KiB 312.53 KiB
8468b08 20.51 KiB 333.04 KiB 312.53 KiB
e9ec817 20.51 KiB 333.11 KiB 312.60 KiB
137e8fe 20.51 KiB 333.11 KiB 312.60 KiB

@@ -31,7 +31,6 @@ - (void)testSpanFinishesAfterTracerReleased_NoCrash_TracerIsNil
}

XCTAssertNotNil(child);
XCTAssertNil(child.tracer);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the child is now a NoOp span, there is no tracer (and you get an unrecognized selector failure).

* master:
  meta: Changelog link start-up crashes (#2238)
  ci: Point to latest GH hash for VLC tests (#2237)
  ci: Fix typo in comments for tests.yml (#2241)
  release: 7.27.0
  feat: Support tracePropagationTargets (#2217)
(this is such an annoying manual thing to keep fixing)
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.

The Span should only return a NoOp if the transaction is finished, not itself.

@brustolin
Copy link
Contributor

The Span should only return a NoOp if the transaction is finished, not itself.

If this is the requirement, we only need to check this at SentryTracer, since is the only place we actually create the child.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@kevinrenskers kevinrenskers merged commit 4a0c282 into master Oct 3, 2022
@kevinrenskers kevinrenskers deleted the fix/1624-finished-span-start-child branch October 3, 2022 08:47
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.

Return NoOpSpan when starting a child on a finished transaction
3 participants