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

Integrate Spotlight #3166

Merged
merged 21 commits into from
Feb 20, 2024
Merged

Integrate Spotlight #3166

merged 21 commits into from
Feb 20, 2024

Conversation

markushi
Copy link
Member

@markushi markushi commented Jan 25, 2024

📜 Description

Add beforeSendEnvelope callback similar to JS and utilize it to send envelopes to spotlight. 🔥
image

💡 Motivation and Context

Fixes #3097

Better DX

💚 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

Copy link
Contributor

github-actions bot commented Jan 25, 2024

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

Generated by 🚫 dangerJS against 9e1abf3

Copy link
Contributor

github-actions bot commented Jan 25, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 407.92 ms 466.29 ms 58.37 ms
Size 1.70 MiB 2.28 MiB 587.88 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
1ae2ec6 353.85 ms 419.71 ms 65.85 ms
99a51e2 405.11 ms 479.65 ms 74.54 ms
1f3652d 361.62 ms 424.76 ms 63.14 ms
86f0096 371.86 ms 439.78 ms 67.92 ms
8fd337b 349.16 ms 459.22 ms 110.06 ms
8ff8fd0 432.77 ms 495.18 ms 62.41 ms
7eccfdb 366.98 ms 440.27 ms 73.29 ms
95a98b5 379.14 ms 420.80 ms 41.66 ms
d6d2b2e 463.14 ms 545.56 ms 82.42 ms
8838e01 367.65 ms 447.40 ms 79.75 ms

App size

Revision Plain With Sentry Diff
1ae2ec6 1.70 MiB 2.27 MiB 584.63 KiB
99a51e2 1.72 MiB 2.29 MiB 576.34 KiB
1f3652d 1.72 MiB 2.27 MiB 554.95 KiB
86f0096 1.72 MiB 2.29 MiB 576.50 KiB
8fd337b 1.72 MiB 2.27 MiB 555.00 KiB
8ff8fd0 1.72 MiB 2.27 MiB 558.15 KiB
7eccfdb 1.72 MiB 2.27 MiB 556.93 KiB
95a98b5 1.70 MiB 2.27 MiB 586.31 KiB
d6d2b2e 1.72 MiB 2.27 MiB 555.05 KiB
8838e01 1.72 MiB 2.29 MiB 578.15 KiB

Previous results on branch: feat/spotlight

Startup times

Revision Plain With Sentry Diff
9fee441 426.64 ms 509.65 ms 83.01 ms
13edfa9 388.96 ms 461.32 ms 72.36 ms
bdecf3c 367.85 ms 437.47 ms 69.62 ms
d450692 371.77 ms 432.52 ms 60.75 ms
9601ea2 343.83 ms 421.55 ms 77.72 ms
ac77a10 380.06 ms 411.46 ms 31.40 ms
b3b3995 378.81 ms 430.72 ms 51.90 ms
ad4e9ce 407.10 ms 479.37 ms 72.27 ms

App size

Revision Plain With Sentry Diff
9fee441 1.70 MiB 2.27 MiB 585.63 KiB
13edfa9 1.70 MiB 2.27 MiB 585.59 KiB
bdecf3c 1.70 MiB 2.27 MiB 585.72 KiB
d450692 1.70 MiB 2.27 MiB 585.72 KiB
9601ea2 1.70 MiB 2.27 MiB 585.63 KiB
ac77a10 1.70 MiB 2.27 MiB 584.47 KiB
b3b3995 1.70 MiB 2.27 MiB 587.65 KiB
ad4e9ce 1.70 MiB 2.27 MiB 585.72 KiB

Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

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

I'm able to run the SpotlightIntegration for Spring Boot 3 by moving it to SentryOptions as suggested in one of the comments but no errors / transactions / ... show up in spotlight. It only shows the SDK on the SDKs tab, nothing else.

sentry/src/main/java/io/sentry/SentryClient.java Outdated Show resolved Hide resolved

private @NotNull SentryOptions options = SentryOptions.empty();

private final @NotNull ExecutorService executorService = Executors.newSingleThreadExecutor();
Copy link
Member

Choose a reason for hiding this comment

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

h can we use executor service from options? This one doesn't set daemon and thus will likely block shutdown for backend (#2747)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've switched to using our SentryExecutorService, which is setup in daemon mode. I still wouldn't use the executor service from our options, as it could slow down any disk/cpu bound tasks when many envelopes are queuing up.

Copy link
Member Author

Choose a reason for hiding this comment

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

@adinauer would the mentioned SentryExecutorService work fine for java backend use cases?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean the one from options or a new one? Either should work. Is there a specific reason you're asking?
Can't we increase the number of threads SentryExecutorService is allowed to use in case we run into problems?

There's Executors.newScheduledThreadPool which we could pass a pool size to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Afaik we can't increase the pool size for options.executorService. At least not on Android, right @romtsn? IIRC on SDK init we rely on some background tasks to be executed in a certain order.

@adinauer adinauer mentioned this pull request Feb 5, 2024
markushi and others added 2 commits February 12, 2024 13:17
….xml

Co-authored-by: Roman Zavarnitsyn <rom4ek93@gmail.com>
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!

@markushi markushi marked this pull request as ready for review February 19, 2024 09:41
@markushi markushi merged commit 536c1b1 into main Feb 20, 2024
24 checks passed
@markushi markushi deleted the feat/spotlight branch February 20, 2024 12:34
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.

Spotlight support
5 participants