relay icon indicating copy to clipboard operation
relay copied to clipboard

feat: migrate to pyo3

Open anonrig opened this issue 1 year ago • 4 comments

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_response does not return uuid as Uuid type, but returns as a string. This is due to the limitation of pyo3.
  • SecretKey does not have a function called pack. This is due to the limitation of pyo3, since it requires json.dumps to be called on an unknown JSON object. The lack of object signature knowledge results in the requirement of calling json.dumps on Python side.
  • relay_store_normalizer_normalize_event requires an object of Annotated<Event> to be deserialized and serialized. Unfortunately, pyo3 doesn't support structs with generics. Therefore, deserializer/serializer can not be used, and normalize_event can not be implemented in a relatively easy way. This applies to several other functions such as pii_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_selector usage

anonrig avatar May 02 '24 20:05 anonrig

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.

anonrig avatar May 15 '24 20:05 anonrig

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 avatar May 31 '24 14:05 loewenheim

@loewenheim any next steps planned for this?

jjbayer avatar Jun 24 '24 08:06 jjbayer

@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.

loewenheim avatar Jun 26 '24 09:06 loewenheim