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

Return transaction for Sentry.getSpan when global hub mode is enabled #2855

Merged
merged 14 commits into from
Aug 31, 2023

Conversation

markushi
Copy link
Member

📜 Description

Fixes #1828

💡 Motivation and Context

Just like Cocoa, always return the root transaction instead of the last active span. This is only applied if globalHubMode is enabled.

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

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

Generated by 🚫 dangerJS against a1914e9

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 358.48 ms 361.78 ms 3.30 ms
Size 1.72 MiB 2.26 MiB 547.97 KiB

Previous results on branch: feat/get-span-root-span

Startup times

Revision Plain With Sentry Diff
015f93f 314.22 ms 348.98 ms 34.75 ms
bfee4ed 342.45 ms 369.92 ms 27.47 ms
e9072f6 295.55 ms 371.12 ms 75.57 ms
3d502fb 316.02 ms 358.81 ms 42.79 ms
6cd0641 312.44 ms 359.16 ms 46.72 ms
9ef1fa6 324.19 ms 374.00 ms 49.81 ms
e068d53 307.73 ms 340.84 ms 33.11 ms

App size

Revision Plain With Sentry Diff
015f93f 1.72 MiB 2.26 MiB 547.97 KiB
bfee4ed 1.72 MiB 2.29 MiB 575.30 KiB
e9072f6 1.72 MiB 2.29 MiB 575.26 KiB
3d502fb 1.72 MiB 2.29 MiB 576.58 KiB
6cd0641 1.72 MiB 2.26 MiB 547.97 KiB
9ef1fa6 1.72 MiB 2.29 MiB 576.28 KiB
e068d53 1.72 MiB 2.29 MiB 575.30 KiB

@romtsn
Copy link
Member

romtsn commented Jul 25, 2023

I think simply changing this on the Sentry.java level is probably not enough, because we are using Hub.getSpan() in our own auto-instrumentation, and those will still be returning the latest span and not the root one.
image

Also, we've talked about introducing another method getLatestSpan or something like that, which we can use in our DB instrumentation to not break the parent-child relationship for Room+SQLite spans, can we also do that in this PR please?

@markushi
Copy link
Member Author

markushi commented Jul 26, 2023

I think simply changing this on the Sentry.java level is probably not enough, because we are using Hub.getSpan() in our own auto-instrumentation, and those will still be returning the latest span and not the root one. image

As of now globalhub mode is not stored anywhere but Sentry.java so the hub itself doesn't know if it's global or not - let me look into this.

Also, we've talked about introducing another method getLatestSpan or something like that, which we can use in our DB instrumentation to not break the parent-child relationship for Room+SQLite spans, can we also do that in this PR please?

Yes, we thought about this, on it...

@romtsn
Copy link
Member

romtsn commented Jul 26, 2023

As of now globalhub mode is not stored anywhere but Sentry.java so the hub itself doesn't know if it's global or not - let me look into this.

We could just use hub.getTransaction() in our integrations maybe, where applicable

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we also have to do it for Apollo, at least on Android? Is the apollo integration used server-side as well @adinauer? But we have the same problem for the okhttp integration now anyway, if someone is using it on the server alongside the Java SDK

@adinauer
Copy link
Member

adinauer commented Aug 9, 2023

Is the apollo integration used server-side as well

It's a client only, we have support for graphql-java on server side which is used by Netflix DGS as well as spring-graphql.

Or do you mean can this be used in a backend application? That's a yes.

Not really sure about details here, maybe we can do a quick call if you need my input here so I can better grasp implications.

@markushi
Copy link
Member Author

markushi commented Aug 9, 2023

Let's change the Sentry.getSpan() behavior for Android only. In case this comes up for Desktop/Cli let's deal with this at a later point.

@romtsn
Copy link
Member

romtsn commented Aug 30, 2023

@markushi I think the PR is still missing SentryApolloInterceptor and SentryApollo3Interceptor from the list 😅
image

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

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

looks good now, thanks for addressing back-and-forths 🎉

@markushi markushi merged commit cd268a3 into feat/7.0.0 Aug 31, 2023
17 checks passed
@markushi markushi deleted the feat/get-span-root-span branch August 31, 2023 12:15
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.

None yet

3 participants