Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(replays): Leave non-critical fields as generic "Value" types #1702

Merged
merged 3 commits into from
Jan 9, 2023

Conversation

cmanallen
Copy link
Member

closes: https://github.com/getsentry/replay-backend/issues/239

Leaves non-critical fields as generic Value types which can be re-serialized later without consequence to PII scrubbing.

@cmanallen cmanallen requested a review from a team December 15, 2022 20:35
Copy link
Contributor

@iker-barriocanal iker-barriocanal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't we losing the type safety by setting everything as Value? What's the problem we're facing here?

@cmanallen
Copy link
Member Author

@iker-barriocanal Type safety is not a requirement. We have no desire to validate the payload. We're parsing and stripping PII. If we could guarantee no PII then we would not parse this at all.

@cmanallen
Copy link
Member Author

@iker-barriocanal The problem we're facing is that these values may not be well typed in their implementation. It's better to just accept values we don't care about and let our javascript implementation sort out the quirks.

@jan-auer
Copy link
Member

@cmanallen is there a desired target type for all these fields? For example, SDKs might send an identifier as either string or number, but you always stringify it for your backend (can follow up with an example). That way, you reduce the burden on all other systems that touch this data to handle all permutations or edge-cases. We noticed with the event protocol in the past that leaving completely untyped and not enforcing constraints early on leads to much added complexity down the road.

If you need an urgent bugfix, we can still merge this. A conversation on the role of normalization and schemas will definitely take a little more time.

@cmanallen
Copy link
Member Author

@jan-auer

is there a desired target type for all these fields?

No. The contract is maintained between the recording component and the player component. Both are open source projects outside the control of Sentry. The rrweb maintainers bear the burden of their schema definitions.

@jan-auer
Copy link
Member

In that case it would probably be better if we define no schema at all and use a plain Annotated<Value> instead. You can still run a generic scrubber on the basic types (object, string, int) without further knowledge of any schema.

@cmanallen
Copy link
Member Author

Let's leave the schema in place for now. I'll review the generic schema implementation independently.

@cmanallen cmanallen merged commit daf7192 into master Jan 9, 2023
@cmanallen cmanallen deleted the replays-stronger-recording-parser branch January 9, 2023 15:30
olksdr pushed a commit that referenced this pull request Jan 11, 2023
)

closes: getsentry/replay-backend#239

Leaves non-critical fields as generic `Value` types which can be
re-serialized later without consequence to PII scrubbing.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants