feat(replays): Use Annotated struct definition for replay-event parsing

[cmanallen]

Removes direct dependency on Serde and replaces it with Annotated. The motivation to move to Annotated was to maintain strong compatibility with wider Sentry ecosystem and to streamline the implementation of PII scrubbing.

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

[github-actions]

	Fails
🚫	Please consider adding a changelog entry for the next release.

Instructions and example for changelog

For changes exposed to the Python package, please add an entry to py/CHANGELOG.md. This includes, but is not limited to event normalization, PII scrubbing, and the protocol.

For changes to the Relay server, please add an entry to CHANGELOG.md under the following heading:

1. Features: For new user-visible functionality.
2. Bug Fixes: For user-visible bug fixes.
3. Internal: For features and bug fixes in internal operation, especially processing mode.

To the changelog entry, please add a link to this PR (consider a more descriptive message):

- Use Annotated struct definition for replay-event parsing. ([#1582](https://github.com/getsentry/relay/pull/1582))

If none of the above apply, you can opt out by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 92c49ea

[cmanallen]

@jan-auer or @jjbayer Having a tough time finishing this one. Trying to figure out the best place to put things and also how all these things fit together.

Goal: normalize the replay event payload and provide data scrubbing functionality that is configurable from sentry.io.

I have a process_replay method defined on the impl for NormalizeProcessor. I'm doing platform normalization there. In actors/processor.rs I'm normalizing the user-agent and the user's ip-address. The ip-address comes from envelope.meta().client_addr();. I'm doing this normalization process in two locations and I'm thinking they should be merged into a single method.

Questions: How should NormalizeProcessor.process_replay be called and should I call it in the process_replays method in actors/processors.rs? How do I access client_addr() inside the normalization processor? Should I even be using the normalization processor?

[jjbayer]

Should I even be using the normalization processor?

@cmanallen haven't reviewed in detail, but I think it might make more sense to write a separate ReplayProcessor (or similar) that implements Processor and normalizes Replay events. It might be possible to reuse the existing PiiProcessor for Pii processing though, by adding a process_replay method to it.

To pass the client IP, you could give your processor a config similar to StoreConfig, which contains client information and is passed into the StoreProcessor:

relay/relay-general/src/store/mod.rs

Lines 33 to 35 in 4fc8084

	pub struct StoreConfig {
	pub project_id: Option<u64>,
	pub client_ip: Option<IpAddr>,

[cmanallen]

@jjbayer Updated with PII scrubbing. Question: the PiiProcessor is removing the user's ip-address (as I expect) but its not removing a credit-card in the tags. Is this unsupported or have I failed to define the correct behavior for this?

Edit: also I did not follow this advice:

It might be possible to reuse the existing PiiProcessor for Pii processing though, by adding a process_replay method to it.

I did not know how process_replay would be called and wasn't really sure what to populate it with. Thankfully process_value with the unmodified PiiProcessor seems to partially work. I wonder what else I need to tweak to get to 100%?

[cmanallen]

@jjbayer Also the linter is freaking out because it says my imports are unnecessary but if I remove them the tests break.

[jjbayer]

@cmanallen is this PR ready for final review? Or are you still making changes?

[cmanallen]

@jjbayer Yes ready for final review! Question: the PII scrubber removes values and replaces them with [email] (for example). On the Snuba side I assume I would want to nullify these values? Do you know what pattern Sentry uses for managing these scrubbed values? I looked at some of the snuba processors and it seems like they don't acknowledge these sentinel values exist. Are there additional steps I need to take to normalize this payload?

[jjbayer]

Do you know what pattern Sentry uses for managing these scrubbed values?

As far as I know, Sentry & Snuba do not treat scrubbed values differently from regular values, i.e. they are still strings, just with a different value. If you'd rather have these values set to null instead of being overwritten, you could look into using the Remove redaction instead of Replace. E.g. by configuring @ip:remove instead of @ip:replace (which is part of @common):

relay/relay-general/src/pii/builtin.rs

Lines 103 to 106 in 4cb923f

	"@ip:remove" => RuleSpec {
	ty: RuleType::Ip,
	redaction: Redaction::Remove,
	};

[cmanallen]

@jjbayer Currently I'm getting the PiiConfig from ProjectConfig? Would using ip@remove require implementing a static PII config?

[jjbayer]

@jjbayer Currently I'm getting the PiiConfig from ProjectConfig? Would using ip@remove require implementing a static PII config?

@cmanallen the short answer is yes.

Do I understand correctly that in this version, replay events are scrubbed with PiiConfig from project config but replay recordings are still scrubbed with a static config? Was it a deliberate product decision to use the one from project configs here? If not, I would apply a static config to both, especially when we need to erase fields to null rather than doing the default scrubbing.

[cmanallen]

@jjbayer We're happy with the configured behavior ([ip] instead of null). Is this ready to be approved?

[jjbayer]

When we fixed this test, I did not realize you were planning to read PII config from project configs instead of using a hard-coded one. So it might make sense to change this test back to a pii config which looks like the most common datascrubbing settings coming from sentry:

relay/relay-general/src/pii/convert.rs

Lines 196 to 203 in e87b92e

	fn simple_enabled_config() -> DataScrubbingConfig {
	DataScrubbingConfig {
	scrub_data: true,
	scrub_ip_addresses: true,
	scrub_defaults: true,
	..Default::default()
	}
	}

But with that, even the credit card scrubbing seems to fail and I'm not sure why. I can investigate later this week.

[jjbayer]

Looks good to me! Could you please add a PR description including a motivation for this change?

[cmanallen]

@jjbayer Updated description. Thanks for helping me through this marathon of a PR! :D