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

Transaction pipeline should normalize the event.spans.data field #4777

Closed
3 tasks done
marandaneto opened this issue Mar 24, 2022 · 6 comments
Closed
3 tasks done

Transaction pipeline should normalize the event.spans.data field #4777

marandaneto opened this issue Mar 24, 2022 · 6 comments

Comments

@marandaneto
Copy link

marandaneto commented Mar 24, 2022

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which package are you using?

@sentry/browser

SDK Version

6.17.9

Framework Version

No response

Link to Sentry event

No response

Steps to Reproduce

Create a transaction and/or span.
Set non-serializable or circular reference values to the span.data and span.tags fields.
Finish the transaction.

Related issue getsentry/sentry-react-native#1477

Expected Result

Non-serializable data or circular references should be removed.
Transaction is captured.

Actual Result

The actual result depends on the broken input as data, e.g. getsentry/sentry-react-native#1477

@marandaneto marandaneto changed the title Transaction pipeline should normalize the transaction.spans.data field Transaction pipeline should normalize the event.spans.data field Mar 24, 2022
@HazAT
Copy link
Member

HazAT commented Jan 25, 2023

We are doing some pii, normalization tasks, closing this for now.

@HazAT HazAT closed this as completed Jan 25, 2023
@marandaneto
Copy link
Author

Unless it changed/got fixed, this is still a bug, the normalization task was not going thru the event.spans.data and tags and would cause errors, such as dropping the event altogether or causing serialization/deserialization issues on the Native layer.
The reason is that we concatenate the data from eg routers and append to the captured transaction/span in order to give more meaningful data to the user, this field isn't guaranteed to be normalized already, so instead of going and fixing every integration, it'd make more sense to actually fix in the normalization task in the JS SDK, avoiding duplication and more source for bugs.
We also did some improvements around that on the SDK a while ago, not sure if it also fixes this use case.
@krystofwoldrich would you like to double-check if this got fixed with the latest versions of the JS SDK?

@HazAT
Copy link
Member

HazAT commented Jan 26, 2023

Thanks for the additional context, I am going to reference the RFC about PII
getsentry/rfcs#62

@HazAT HazAT reopened this Jan 26, 2023
@krystofwoldrich
Copy link
Member

@marandaneto Yes, I will take a look.

@krystofwoldrich
Copy link
Member

data are normalized
tags are not
But by the typings tags can be always serializable. But It would definitely be safer to normalize them too.

@AbhiPrasad
Copy link
Member

Going to close this as I think we've address it with v8. Please re-open if it still applies!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants