Skip to content
This repository was archived by the owner on Aug 30, 2023. It is now read-only.

Set SdkVersion in default SentryOptions created in sentry-core module#506

Merged
bruno-garcia merged 6 commits into
masterfrom
core-sdk-version
Aug 5, 2020
Merged

Set SdkVersion in default SentryOptions created in sentry-core module#506
bruno-garcia merged 6 commits into
masterfrom
core-sdk-version

Conversation

@maciejwalkowiak
Copy link
Copy Markdown
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Sets SdkVersion in SentryOptions created in sentry-core module from build-time generated class BuildConfig.

💡 Motivation and Context

getsentry/sentry-java#873

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing
  • No breaking changes

Comment thread sentry-core/build.gradle.kts
Comment thread sentry-core/build.gradle.kts Outdated
buildConfig {
useJavaOutput()
packageName("io.sentry.core")
buildConfigField("String", "SDK_NAME", "\"${project.name}\"")
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It would be sentry-core. Ok, I'll change it to sentry.java.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe the key of this field should be SENTRY_CLIENT_NAME so it matches options.sentryClientName

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually I missed here setting options.sentryClientName - at this moment we are just setting SdkVersion. I will align it with Android implementation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah wait wait, I've mistaken the topic maybe too.
sentryClientName != the sdkVersion.packages.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think you're right in the end. I'll add this line to SentryOptions.

setSentryClientName(BuildConfig.SENTRY_CLIENT_NAME + "/" + BuildConfig.VERSION_NAME);

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia Aug 4, 2020

Choose a reason for hiding this comment

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

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

Comment thread buildSrc/src/main/java/Config.kt Outdated
Copy link
Copy Markdown
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Just a couple of things but we're almost good to merge

Comment thread buildSrc/src/main/java/Config.kt Outdated
Comment thread buildSrc/src/main/java/Config.kt Outdated
Comment thread sentry-core/src/test/java/io/sentry/core/SentryOptionsTest.kt
@maciejwalkowiak
Copy link
Copy Markdown
Contributor Author

@bruno-garcia take please another look if there is anything pending.

@bruno-garcia bruno-garcia merged commit a896e7a into master Aug 5, 2020
@bruno-garcia bruno-garcia deleted the core-sdk-version branch August 5, 2020 14:53
@bruno-garcia
Copy link
Copy Markdown
Member

Thanks @maciejwalkowiak !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants