fix(core): Ignore non-primitive values passed to setTag(s)
#18167
Conversation
| this._tags = { ...this._tags, [key]: value }; | ||
| this._notifyScopeListeners(); | ||
| return this; | ||
| return this.setTags({ [key]: value }); |
There was a problem hiding this comment.
small simplification here which reduces the bundle size hit from the additional check in setTags
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.
|
Lms24
force-pushed
the
lms/fix-core-settag-non-primitive
branch
from
709a5e2 to
b56bece
Compare
Lms24
requested review from
a team,
AbhiPrasad,
RulaKhaled and
andreiborza
and removed request for
a team and
RulaKhaled
andreiborza
left a comment
andreiborza
left a comment
There was a problem hiding this comment.
LGTM although if the product can handle it and nobody complained about this yet, I wonder if the bundle-size hit is even worth it. Are there any other concerns with this?
That's a fair point. At least no-one complained about this yet, so we can also just leave things-as is. As written in the issue, typing-wise, we disallow non-primitives anyway. CDN bundle and plain JS users just likely don't get the type errors if they add anything else. I'll close this for now and follow up with a small PR to clarify the situation in the test and add that minimal bundle size improvement + tests to setTag |
Previously, the SDK would accept and send non-primitive tag values, despite this violating the tag specification as well as our documentation around the API. While the Sentry product can handle it (it displays the tag as invalid), I believe we should still avoid sending it in the first place. Especially because we also had a faulty test that asserted that non-primitive values were ignored.
closes #18163