fix: event SDK info should depend on the configured options

[vaind]

📜 Description

So this is yet another follow up to setting SDK info through options (related to #1793 and #1816). I've incorrectly assumed the option is actually being used... turns out it's not and other parts of the code still use SentryMeta.sdkName/version directly. This PR is the minimum fix (with some minor performance-related refactoring of the setSdk(event) on the go).

There are other places where SentryMeta is used and IMO shouldn't, most importantly a couple of times in SentryEnvelopeHeader - I could have a look at that one too if needed, but this PR seems to already get the right info to sentry.io

💡 Motivation and Context

So the events actually show up in the UI with the right SDK.

💚 How did you test it?

Manually. Is there a test case I should update?

📝 Checklist

• I reviewed the submitted code
• I added tests to verify the changes
• I updated the docs if needed
• Review from the native team if needed
• No breaking changes

🔮 Next steps

[codecov-commenter]

Codecov Report

Merging #1853 (f021439) into master (5543879) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
+ Coverage   92.13%   92.21%   +0.07%     
==========================================
  Files         200      200              
  Lines        9450     9464      +14     
==========================================
+ Hits         8707     8727      +20     
+ Misses        743      737       -6     

Impacted Files	Coverage Δ
Sources/Sentry/SentryClient.m	99.43% <100.00%> (+0.28%)	⬆️
Sources/Sentry/SentryTransportAdapter.m	100.00% <100.00%> (ø)
Sources/Sentry/SentryRequestOperation.m	69.23% <0.00%> (-7.70%)	⬇️
Sources/Sentry/SentryMachLogging.cpp	2.05% <0.00%> (ø)
Sources/Sentry/SentryTracer.m	98.94% <0.00%> (+0.26%)	⬆️
Sources/Sentry/SentryBacktrace.cpp	95.31% <0.00%> (+1.56%)	⬆️
Sources/Sentry/SentryThreadHandle.cpp	73.20% <0.00%> (+1.96%)	⬆️
Sources/Sentry/SentryProfilingLogging.mm	60.71% <0.00%> (+7.14%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5543879...f021439. Read the comment docs.

[bruno-garcia]

The value from SentryMeta is added to envelope headers so Relay can also know what SDK is sending the data to its backend. This can help us troubleshoot issues where one SDK is sending bad data.

Ideally we find a way to change that value and affect all corners of the SDK, instead of reporting a different value here and on the envelope header, for example.

[vaind]

Ideally we find a way to change that value and affect all corners of the SDK, instead of reporting a different value here and on the envelope header, for example.

I completely agree and I've had another look and updated all code paths from the actual SDK code (at least I think I've done all of them). The remaining call paths are either from test code or public APIs that I can't touch without breaking changes.

[vaind]

Would be nice to add/update tests to verify the configured SDK override is actually propagated to events and envelopes, but I really don't know which test would be a good candidate to update. Maybe someone from the maintainers could have a look and make the change, or at least point me to the appropriate test case.

[philipphofmann]

You discovered a flaw in our SDK that is not easy to fix 😁. SentryOptions should be the single source of truth of the SDK info. When initializing the SentryOptions, we can read the default values from SentryMeta, but we should always use the SentryOptions for getting the SDK info and not the SentryMeta. One major problem is that SentryEnvelopeHeader uses SentryMeta

sentry-cocoa/Sources/Sentry/SentryEnvelope.m

Lines 18 to 44 in 4ec83c3

	@implementation SentryEnvelopeHeader
	// id can be null if no event in the envelope or attachment related to event
	- (instancetype)initWithId:(SentryId *_Nullable)eventId
	{
	SentrySdkInfo *sdkInfo = [[SentrySdkInfo alloc] initWithName:SentryMeta.sdkName
	andVersion:SentryMeta.versionString];
	self = [self initWithId:eventId andSdkInfo:sdkInfo];
	return self;
	}
	- (instancetype)initWithId:(SentryId *_Nullable)eventId andSdkInfo:(SentrySdkInfo *_Nullable)sdkInfo
	{
	return [self initWithId:eventId sdkInfo:sdkInfo traceState:nil];
	}
	- (instancetype)initWithId:(nullable SentryId *)eventId
	traceState:(nullable SentryTraceState *)traceState
	{
	SentrySdkInfo *sdkInfo = [[SentrySdkInfo alloc] initWithName:SentryMeta.sdkName
	andVersion:SentryMeta.versionString];
	self = [self initWithId:eventId sdkInfo:sdkInfo traceState:traceState];
	return self;
	}

The initializers of SentryEnvelopeHeader are public, and so are the ones of SentryEnvelopes. 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.

What do you think @vaind, @bruno-garcia, and @brustolin?

[vaind]

Thanks for the wrapup - yes, that's what I've gathered when working on this PR.

To fix this, we have to add new initializers to SentryEnvelopeHeader and SentryEnvelope requiring the SDKInfo

The necessary initializers are already there and, after the changes in this PR, in use for all events & envelopes created by the SDK (to the best of my knowledge). There're a lot of test-cases that manually initialize the Envelope/Header and don't use the sdkInfo initializers but I guess that's fine.

So yes, a breaking change (Option A) is necessary if we want to make sure users don't manually create envelopes without providing the SDK info. Other than that, for normal use-cases where users just report events/trasactions/etc, this PR should cover the necessary code paths without being a breaking change. So my suggestion would be to go now with this non-breaking change of using the right APIs internally.

Can you maybe suggest/add a test case that would check that the configured options SDK info is correctly passed to captured events, transactions, user feedback & their envelopes? I think that's all that would be needed to land this change.

[philipphofmann]

Most users shouldn't have to use SentryEnvelope. I would go with Option A. We make sure that we use the right APIs internally, add new initializers, and mark the other initializers as deprecated.

There is no good test case yet to ensure that the right SDKInfo is passed to captured events, and transactions because this information was previously static. Every envelope got the info from SentryMeta. Ensuring that we only call the new constructors with the right SDKInfo is the best test I can think of. Then we can add a few tests for SentryClient, SentryTransport, SentryTransportAdapter, basically where we create SentryEnvelopes, to ensure these use the correct SDKInfo from the options. Maybe I would come up with a better idea if I would implement this. Do you have better suggestions?

[vaind]

Maybe I would come up with a better idea if I would implement this.

I'd be happy if you could take over the test cases because I'm not an objective-c/swift developer.

[philipphofmann]

I'd be happy if you could take over the test cases because I'm not an objective-c/swift developer.

Sure, how urgently do you need this, @vaind?

[vaind]

I'd be happy if you could take over the test cases because I'm not an objective-c/swift developer.

Sure, how urgently do you need this, @vaind?

@bruno-garcia can answer that.

Also, just from the functional perspective, it would be OK to review & merge the code changes in this PR and add tests, which require a bit more work, in a follow-up one, if you don't have time in the near future. Of course, that's technical-debt accumulation.

[philipphofmann]

Also, just from the functional perspective, it would be OK to review & merge the code changes in this PR and add tests, which require a bit more work, in a follow-up one, if you don't have time in the near future. Of course, that's technical-debt accumulation.

I'm not too worried about the tests, just a few should be fine, but this could be a refactoring touching multiple places in the code. I just need a couple of hours to get this done, I think.

[vaind]

I was just saying that the existing code in the PR already does all that's needed (as tested manually in sentry-unity) to the best of my knowledge. And there are no breaking changes

[philipphofmann]

I was just saying that the existing code in the PR already does all that's needed (as tested manually in sentry-unity) to the best of my knowledge. And there are no breaking changes

That's true. Let's merge this and I create an issue to properly fix this.

[philipphofmann]

LGTM 🚀