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.

Sign up for GitHub

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

Jump to bottom

feat: Send SDK Integration List, add missing packages #2179

Merged
merged 44 commits into from
Feb 21, 2023
Merged

feat: Send SDK Integration List, add missing packages #2179

merged 44 commits into from
Feb 21, 2023

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jul 20, 2022 •
edited by lbloder

📜 Description

  • Changed List to Set because we don't care about duplicates (and also, it's easier to use, e.g. for Navigation listener, which can be attached/detached multiple times during the app lifetime)
  • It's also a synchronized Set since it can be modified from different threads
  • Keep List getter as to not introduce breaking changes
  • An easier would be to change the signature of register method which would return true/false depending on whether the integration is enabled or not, then we could add all of them on the registring site. But this would mean breaking the public API so I opted out from that
  • Adapt/add tests
  • Introduced SentryIntegrationPackageStorage Singleton to correctly handle Java integrations like log4J, logback and OTEL that might initialize Sentry but get overriden by e.g. Spring Boot.
  • Introduced IntegrationName interface

outdated

You can see the event with integrations here https://sentry.io/organizations/sentry-sdks/issues/3402041535/events/e085890223004873b274c7bb6c6d1c8f/json/

💡 Motivation and Context

Closes #1885

🔮 Next steps

@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2022 •
edited

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

Generated by 🚫 dangerJS against 594cc5e

lbloder and others added 3 commits December 14, 2022 16:00
@lbloder
Merge branch 'main' into feat/integrations-in-events
006a968 
# Conflicts:
#	sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
#	sentry-android-core/src/main/java/io/sentry/android/core/AppLifecycleIntegration.java
#	sentry/src/main/java/io/sentry/UncaughtExceptionHandlerIntegration.java
@lbloder
add java integrations to integration list
3d261c1
@getsentry-bot
Format code
0ed5f76
@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2022 •
edited

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 324.45 ms 380.57 ms 56.12 ms
Size 1.73 MiB 2.34 MiB 626.30 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4b32504 315.69 ms 373.96 ms 58.27 ms
754021c 358.70 ms 361.98 ms 3.28 ms
fe30606 310.82 ms 335.36 ms 24.55 ms
14c083a 350.82 ms 388.86 ms 38.04 ms
4b32504 357.14 ms 404.04 ms 46.90 ms
c1399c1 345.06 ms 385.49 ms 40.43 ms
f6a135d 263.96 ms 383.59 ms 119.63 ms
5fa24ec 326.29 ms 384.53 ms 58.24 ms
efa7b3d 330.06 ms 370.14 ms 40.08 ms
efa7b3d 335.38 ms 384.85 ms 49.48 ms

App size

Revision Plain With Sentry Diff
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
754021c 1.73 MiB 2.33 MiB 623.06 KiB
fe30606 1.73 MiB 2.34 MiB 623.74 KiB
14c083a 1.73 MiB 2.33 MiB 620.61 KiB
4b32504 1.73 MiB 2.34 MiB 623.74 KiB
c1399c1 1.73 MiB 2.33 MiB 620.61 KiB
f6a135d 1.73 MiB 2.33 MiB 623.10 KiB
5fa24ec 1.73 MiB 2.33 MiB 620.61 KiB
efa7b3d 1.73 MiB 2.34 MiB 625.56 KiB
efa7b3d 1.73 MiB 2.34 MiB 625.56 KiB

Previous results on branch: feat/integrations-in-events

Startup times

Revision Plain With Sentry Diff
607bde5 312.84 ms 341.02 ms 28.18 ms
f2ac750 331.21 ms 382.12 ms 50.92 ms
47b5673 317.63 ms 363.37 ms 45.74 ms
13749c3 335.81 ms 367.06 ms 31.25 ms
3556d6d 273.42 ms 318.63 ms 45.22 ms
20115be 339.14 ms 381.59 ms 42.44 ms
9668485 347.31 ms 370.49 ms 23.18 ms
8789ba4 355.11 ms 394.53 ms 39.42 ms
6b688da 374.96 ms 422.16 ms 47.20 ms
d1e381e 300.51 ms 369.57 ms 69.06 ms

App size

Revision Plain With Sentry Diff
607bde5 1.73 MiB 2.33 MiB 621.17 KiB
f2ac750 1.73 MiB 2.34 MiB 626.06 KiB
47b5673 1.73 MiB 2.34 MiB 626.08 KiB
13749c3 1.73 MiB 2.33 MiB 621.17 KiB
3556d6d 1.73 MiB 2.33 MiB 614.83 KiB
20115be 1.73 MiB 2.34 MiB 626.01 KiB
9668485 1.73 MiB 2.34 MiB 626.03 KiB
8789ba4 1.73 MiB 2.34 MiB 626.08 KiB
6b688da 1.73 MiB 2.34 MiB 626.08 KiB
d1e381e 1.73 MiB 2.34 MiB 624.19 KiB

@romtsn
Copy link
Member Author

romtsn commented Dec 29, 2022

@lbloder what do you think of Philipp's suggestion? At least for those that implement the Integration interface, we could get the class name and remove the Integration part from its name when we call register. This would reduce the maintenance overhead for those that implement Integration at least.

@romtsn
Copy link
Member Author

romtsn commented Dec 29, 2022

@lbloder can I also ask you to add the missing integrations to packages (sdkVersion.addPackage())? This would also help us to analyze data based on packages. The modules that need this to be added:

  • okhttp
  • apollo2
  • apollo3
  • fragment
  • navigation
  • compose
  • compose-helper (also needs to be added to the integrations list)

Don't know about jdbc and graphql, since you haven't added them to the integrations yet

adinauer
adinauer requested changes Feb 16, 2023
View reviewed changes
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.

We need to find a different way of handling deserialization. Clearing all packages / integrations would lead to bad data.

CHANGELOG.md Outdated Show resolved Hide resolved
sentry-android-timber/src/main/java/io/sentry/android/timber/SentryTimberIntegration.kt Outdated Show resolved Hide resolved
sentry-android-timber/src/test/java/io/sentry/android/timber/SentryTimberIntegrationTest.kt Show resolved Hide resolved
...ization/src/main/java/io/sentry/opentelemetry/SentryAutoConfigurationCustomizerProvider.java Outdated Show resolved Hide resolved
...ization/src/main/java/io/sentry/opentelemetry/SentryAutoConfigurationCustomizerProvider.java Show resolved Hide resolved
sentry/src/main/java/io/sentry/ShutdownHookIntegration.java Outdated Show resolved Hide resolved
sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt Outdated Show resolved Hide resolved
sentry/src/test/java/io/sentry/SendCachedEnvelopeFireAndForgetIntegrationTest.kt Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/SentryIntegrationPackageStorage.java Outdated Show resolved Hide resolved
sentry/src/main/java/io/sentry/protocol/SdkVersion.java Outdated Show resolved Hide resolved
adinauer
adinauer reviewed Feb 16, 2023
View reviewed changes
CHANGELOG.md Outdated
@@ -16,6 +16,7 @@

- Add time-to-full-display span to Activity auto-instrumentation ([#2432](https://github.com/getsentry/sentry-java/pull/2432))
- Add `main` flag to threads and `in_foreground` flag for app contexts ([#2516](https://github.com/getsentry/sentry-java/pull/2516))
- Add integrations to integration list ([#2179](https://github.com/getsentry/sentry-java/pull/2179))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe put this under Fixes and call it:

Integrations and packages in use are now reported more consistently

wdyt?

romtsn
romtsn commented Feb 16, 2023
View reviewed changes
...-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java Outdated
@@ -19,6 +20,10 @@
@SuppressWarnings("KotlinInternalInJava")
public final class ComposeGestureTargetLocator implements GestureTargetLocator {

public ComposeGestureTargetLocator() {
SentryIntegrationPackageStorage.getInstance().addIntegration("ComposeHelper");
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
SentryIntegrationPackageStorage.getInstance().addIntegration("ComposeHelper");
SentryIntegrationPackageStorage.getInstance().addIntegration("ComposeUserInteraction");
SentryIntegrationPackageStorage.getInstance().addPackage("maven:io.sentry:sentry-compose", BuildConfig.VERSION_NAME)

since it's embedded in sentry-compose

@romtsn
Copy link
Member Author

romtsn commented Feb 16, 2023

One more ask - could you please add SentryIntegrationPackageStorage.getInstance().addIntegration("FileIO"); to the FileIOSpanManager ctor? this way we don't need to wait for gradle plugin to inject its features, but can already track FileIO usages. Thanks 🙏

@lbloder lbloder changed the title feat: initial implementation for sending SDK integrations list feat: Send SDK Integration List, add missing packages Feb 17, 2023
@lbloder lbloder marked this pull request as ready for review February 17, 2023 13:25
adinauer
adinauer approved these changes Feb 21, 2023
View reviewed changes
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.

Just finished this off by fixing compile, adding BuildConfig to compose-helper and adding a test to ensure storage doesn't affect deserialized SdkVersion instances.

@adinauer adinauer merged commit 4694204 into main Feb 21, 2023
@adinauer adinauer deleted the feat/integrations-in-events branch February 21, 2023 09:54
@lbloder lbloder mentioned this pull request Mar 10, 2023
Merged
7 tasks
@romtsn romtsn mentioned this pull request Mar 16, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipphofmann philipphofmann philipphofmann left review comments

@lbloder lbloder lbloder left review comments

@adinauer adinauer adinauer approved these changes

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano is a code owner

@markushi markushi Awaiting requested review from markushi markushi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add SDK integrations to events
5 participants
@romtsn @philipphofmann @adinauer @lbloder @getsentry-bot