feat: migrate to pyo3
Written in collaboration with @loewenheim
Goal
The goal of this pull-request is to remove relay_cabi, relay_ffi and relay_ffi_macros and replace it with a more modern approach: pyo3. The proposed changes reduce the Python surface area of the implementation and implements most of the functionality in Rust, making maintenance a lot easier to the current approach.
Limitations
- pyo3 does not support struct with generics.
Missing functionality:
validate_register_responsedoes not returnuuidas Uuid type, but returns as a string. This is due to the limitation of pyo3.SecretKeydoes not have a function calledpack. This is due to the limitation of pyo3, since it requiresjson.dumpsto be called on an unknown JSON object. The lack of object signature knowledge results in the requirement of callingjson.dumpson Python side.relay_store_normalizer_normalize_eventrequires an object ofAnnotated<Event>to be deserialized and serialized. Unfortunately, pyo3 doesn't support structs with generics. Therefore, deserializer/serializer can not be used, andnormalize_eventcan not be implemented in a relatively easy way. This applies to several other functions such aspii_strip_event.
Todo
- [x] remove relay_cabi
- [x] remove relay_ffi
- [x] remove relay_ffi_macros
- [x] remove py/sentry_relay folder
- [x] update GitHub workflow
- [ ] update py/Dockerfile
- [x] update makefile
- [x] add types for
sentry_relay.validate_pii_selectorusage
Hey @jjbayer, thank you for the quick Look!
Whats the reason for moving the crate to py/rust? I would prefer a top level workspace crate relay-pyo3 or similar (essentially renaming relay-cabi).
There was no specific reason, merely choice. @loewenheim fixed it and moved it into the correct place in the repo.
Should we split this into multiple steps and as a first step introduce Py03 for the bindings without introducing the dependency in many of our crates? E.g. normalize_global_config could be a #[pyfunction] which still calls serde internally. Ideally the function signatures on the python side (including error handling) do not change. This would reduce the risk of rolling this out.
Ideally yes, for now, we're trying to focus on correctness, and meanwhile I'll prepare a document to discuss our options to merge and release this.
StoreNormalizer.normalize_event continues to be a huge problem. It has two parameters, event and raw_event. event is an event dictionary object, raw_event is the serialization thereof. event is only used of raw_event is not used, so it's immediately unclear why they both need to exist—just use one parameter and serialize it if necessary.
Unfortunately it's currently impossible to convert between PyDict and Event, so the only way to pass event dictionaries across the Python/Rust boundary is string serialization.
@loewenheim any next steps planned for this?
@jjbayer There's still some work to be done; I'm a bit busy with other things but I do want to get this over the finish line.