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

Use SentryOptions.sdkInfo instead of SentryMeta #1884

Closed
philipphofmann opened this issue Jun 15, 2022 · 7 comments · Fixed by #1960
Closed

Use SentryOptions.sdkInfo instead of SentryMeta #1884

philipphofmann opened this issue Jun 15, 2022 · 7 comments · Fixed by #1960
Assignees

Comments

@philipphofmann
Copy link
Member

Description

The SDK currently uses both SentryOptions.sdkInfo and SentryMeta for creating the sdkInfo.

To fix this, we have to add new initializers to SentryEnvelopeHeader and SentryEnvelope, requiring the SDKInfo. On Java, we also have to pass in the SDKInfo as a parameter see https://github.com/getsentry/sentry-java/blob/9f1af1c68c58570a731a65289183107d3c4f90d5/sentry/src/main/java/io/sentry/SentryEnvelope.java#L36-L78

We can't get rid of the initializers using SentryMeta, because this would be a breaking change. I think we have two options:

Option A
Marking these initializers as deprecated, adding new ones requiring SDKInfo as a parameter remove them in 8.x.x., but this would bubble up to all initializers of SentryEnvelopes.

Option B
Use SentrySDK.currentHub for getting the options in SentryEnvelope and SentryEnvelopeHeader, and mark the initializers as deprecated in 8.x.x and remove them in 9.x.x. Even if someone would use multiple clients with different options, they most likely will use the same SDKInfo.

This came up in #1853

@brustolin
Copy link
Contributor

I believe we could inject SentryOptions into DependencyContainer to use it across the project

@philipphofmann
Copy link
Member Author

I believe we could inject SentryOptions into DependencyContainer to use it across the project

This won't work if you use multiple hubs and not the static API.

@philipphofmann
Copy link
Member Author

Why is SDKInfo even on the options? Is there a use case for users to change this? One requirement for changing the SDKInfo is for our hybrid SDKs, but they could use a private API instead to override the SentryMeta. @bruno-garcia, @marandaneto are we missing something?

@philipphofmann philipphofmann moved this from Needs Discussion to Needs More Information in Mobile & Cross Platform SDK Jun 22, 2022
@philipphofmann philipphofmann moved this from Needs More Information to Needs Discussion in Mobile & Cross Platform SDK Jun 22, 2022
@bruno-garcia
Copy link
Member

bruno-garcia commented Jun 22, 2022

I believe we could inject SentryOptions into DependencyContainer to use it across the project

This won't work if you use multiple hubs and not the static API.

is Hub public here? I thought multiple Hubs wasn't something we supported.

Happy to use a private API to set it, for as long as we can override the value from things like Unity and possibly more SDKs going forward

@philipphofmann
Copy link
Member Author

is Hub public here? I thought multiple Hubs wasn't something we supported.

Yes, it's public so that you could use multiple hubs. I guess the majority though uses the static API.

@marandaneto
Copy link
Contributor

Why is SDKInfo even on the options? Is there a use case for users to change this? One requirement for changing the SDKInfo is for our hybrid SDKs, but they could use a private API instead to override the SentryMeta. @bruno-garcia, @marandaneto are we missing something?

It's needed for either Hybrid SDKs or when an SDK consists of multiple packages, eg.
By default options.sdkVersion will point to sentry, but if you're using the sentry-spring-boot-starter package, you actually want to overwrite the options.sdkVersion to sentry-spring-boot-starter and adding the sdkVersion to the options was the easiest way, I don't think there are more use cases.

@brustolin brustolin moved this from Needs Discussion to Backlog in Mobile & Cross Platform SDK Jun 29, 2022
@brustolin
Copy link
Contributor

brustolin commented Jun 29, 2022

This information should not be available to the end-user in the options.
We need to add a way in the private Hybrid class (PrivateSentrySDKOnly) to change the SDKInfo,
this information needs to be a unique source of truth in a static class.
Mark the SDKInfo option as deprecated.
Remember to communicate with Unity team (@bitsandfoxes)

kevinrenskers added a commit that referenced this issue Jul 11, 2022
SentryMeta is now the only source of truth. The values can be overridden by hybrid SDKs via SentrySDKOnly.

Closes #1884
@kevinrenskers kevinrenskers moved this from Backlog to In Progress in Mobile & Cross Platform SDK Jul 12, 2022
Repository owner moved this from In Progress to Done in Mobile & Cross Platform SDK Jul 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants