-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Analytics / Telemetry #582
Conversation
|
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Outdated
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/AppConfig.kt
Outdated
Show resolved
Hide resolved
BuildOperationListener, | ||
AutoCloseable { | ||
|
||
private var hub: IHub = NoOpHub.getInstance() |
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.
m
: I believe all of this should be atomic, because it can be accessed by multiple tasks in parallel as mentioned here https://docs.gradle.org/current/javadoc/org/gradle/api/services/BuildService.html
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Outdated
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Outdated
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Outdated
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Show resolved
Hide resolved
plugin-build/src/main/kotlin/io/sentry/android/gradle/telemetry/SentryTelemetryService.kt
Outdated
Show resolved
Hide resolved
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.
Looks good overall, just some comments on the SentryTelemetryService where I wasn't sure about some things, so I haven't changed them myself. 🚀
Oh and maybe one more thing - maybe we should have at least one test that disables telemetry and verifies that the hub is no-op (or just a log message at least) |
📜 Description
We now send telemetry data unless a self hosted
url
has been set. This can be disabled via thetelemetry
option.💡 Motivation and Context
Closes #223
💚 How did you test it?
Manually using samples
📝 Checklist
🔮 Next steps