Set SdkVersion in default SentryOptions created in sentry-core module
#506
Conversation
maciejwalkowiak
requested review from
bruno-garcia and
marandaneto
as code owners
sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java
Outdated
Show resolved
Hide resolved
sentry-core/build.gradle.kts
Outdated
| buildConfig { | ||
| useJavaOutput() | ||
| packageName("io.sentry.core") | ||
| buildConfigField("String", "SDK_NAME", "\"${project.name}\"") |
There was a problem hiding this comment.
project name will be what here exactly? sentry-core? I'm a bit lost but SDK name is sentry.java, sentry.java.android`.. etc. Sentry convention
There was a problem hiding this comment.
It would be sentry-core. Ok, I'll change it to sentry.java.
There was a problem hiding this comment.
maybe the key of this field should be SENTRY_CLIENT_NAME so it matches options.sentryClientName
There was a problem hiding this comment.
Actually I missed here setting options.sentryClientName - at this moment we are just setting SdkVersion. I will align it with Android implementation.
There was a problem hiding this comment.
ah wait wait, I've mistaken the topic maybe too.
sentryClientName != the sdkVersion.packages.
There was a problem hiding this comment.
I think you're right in the end. I'll add this line to SentryOptions.
setSentryClientName(BuildConfig.SENTRY_CLIENT_NAME + "/" + BuildConfig.VERSION_NAME);
There was a problem hiding this comment.
This client name goes into the outgoing HTTP header, right? That, to be honest, doesn't really matter the value/format. We don't use for anything except debuggin (and possibly dropping all events on a load balancer based on this value) but the sdk name is important (i.e: sentry.java, sentry.java.android, sentry.java.spring, etc). And packages, are the actual package in the format pkgmanager/pkgname, like maven:io.sentry, maven:io.sentry.android, nuget:Sentry.Serilog
sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt
Show resolved
Hide resolved
sentry-android-core/src/test/java/io/sentry/android/core/SentryAndroidOptionsTest.kt
Show resolved
Hide resolved
bruno-garcia
left a comment
bruno-garcia
left a comment
There was a problem hiding this comment.
Just a couple of things but we're almost good to merge
|
@bruno-garcia take please another look if there is anything pending. |
|
Thanks @maciejwalkowiak ! |
📢 Type of change
📜 Description
Sets
SdkVersioninSentryOptionscreated in sentry-core module from build-time generated classBuildConfig.💡 Motivation and Context
getsentry/sentry-java#873
📝 Checklist