-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Move onFinishCallback before span or transaction is finished #3459
Conversation
transactionOptions.getTransactionFinishedCallback(); | ||
if (finishedCallback != null) { | ||
finishedCallback.execute(this); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the performanceCollectors here, to fix an issue with SpanFrameMetricsCollection
which was not setting the frame counts measurements, due to transaction being already finished
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 463.14 ms | 545.56 ms | 82.42 ms |
a3c77bc | 375.80 ms | 445.85 ms | 70.06 ms |
c7e2fbc | 377.85 ms | 426.35 ms | 48.50 ms |
f5e1b97 | 362.53 ms | 429.31 ms | 66.78 ms |
7eccfdb | 389.94 ms | 461.29 ms | 71.35 ms |
f1fdb9f | 404.21 ms | 496.87 ms | 92.66 ms |
28c9a83 | 346.14 ms | 377.76 ms | 31.62 ms |
baaf637 | 375.71 ms | 441.58 ms | 65.87 ms |
283d83e | 348.44 ms | 392.06 ms | 43.62 ms |
61981dc | 388.65 ms | 454.96 ms | 66.31 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
d6d2b2e | 1.72 MiB | 2.27 MiB | 555.05 KiB |
a3c77bc | 1.72 MiB | 2.29 MiB | 577.53 KiB |
c7e2fbc | 1.72 MiB | 2.29 MiB | 576.40 KiB |
f5e1b97 | 1.70 MiB | 2.28 MiB | 592.00 KiB |
7eccfdb | 1.72 MiB | 2.27 MiB | 556.93 KiB |
f1fdb9f | 1.70 MiB | 2.28 MiB | 592.08 KiB |
28c9a83 | 1.70 MiB | 2.28 MiB | 592.00 KiB |
baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
283d83e | 1.72 MiB | 2.29 MiB | 577.69 KiB |
61981dc | 1.70 MiB | 2.28 MiB | 592.12 KiB |
…rame measurements not being set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one!
sentry-android-core/src/main/java/io/sentry/android/core/SpanFrameMetricsCollector.java
Outdated
Show resolved
Hide resolved
@@ -37,7 +37,9 @@ public final class Span implements ISpan { | |||
|
|||
private final @NotNull IHub hub; | |||
|
|||
private final @NotNull AtomicBoolean finished = new AtomicBoolean(false); | |||
private boolean finished = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
this probably should be volatile, but then on the other hand we also don't do this for other span properties ¯_(ツ)_/¯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's keep AtomicBoolean
and change it later if ever needed, as i don't think it really changes anything
📜 Description
moved span and transaction finish callback before they are actually finished
💡 Motivation and Context
Closes #3228
This fixes and aligns the behaviour among spans and transactions.
💚 How did you test it?
Unit tests
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps