-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(core): Ensure partially set SDK metadata options are preserved #19102
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
Conversation
Codecov Results 📊Generated by Codecov Action |
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| }; | ||
| } | ||
|
|
||
| if (!metadata.sdk) { |
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.
I might be missing something but isn't this doing the same check as line 20? if so metadata.sdk will always be set before it hits this line here, no?
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.
Yeah not sure why but I had a massive brain outage on this one: I added my new logic underneath the old one and forgot to delete the old one afterwards. 🤦
Found a way to make this all a bit more concise. Might even save us some bytes. 🤞 Thanks for calling this out!
logaretm
left a comment
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.
Fridays be like that, nice one!
Previously, our
applySdkMetadatahelper in core would only apply default SDK metadata (name, packages, version) if nosdkdata at all was set. This caused incomplete SDK metadata when manually overridingsdk.settings(see #19072).This PR now ensures that only if the
nameis already set, we don't apply our default options. This should be fine-grained enough to still have correct SDK metadata inheritance. I also added a bunch of tests, sinceapplySdkMetadatahad no unit tests.To be clear, I don't think we should promote controlling IP inference via SDK metadata. However, for now, it's the only way to control IP inference in a fine-grained manner, as opposed to setting
sendDefaultPii. The long-term solution is a more fine grained Pii SDK option as we're already planning to do. I think that the metadata application now is more correct though, since this also allows other SDKs to override such settings should they need to.closes #19072