Envelope Only Transport API #426
Conversation
|
@bruno-garcia would you mind doing a final review? quite busy these days. |
There was a problem hiding this comment.
Could be beneficial to add some snapshot tests to validate that we're not breaking compatibility with data that's already serialized. The round trip tests are great but have this limitations.
Worth noting this implementation is not keeping track of json data that doesn't map to the type we have and will lose data if we deserialize an envelope written by an SDK on a newer version, with new fields.
Ideally such fields would be kept on a 'bag' and when/if the type gets serialized again, they are written back with the same key as they had.
That said, we attempted to get that on Android when we wrote it but didn't manage, and don't have it with .NET either.
I'm writing this to bring awareness of this situation, not to state that this blocks the PR.
For example, if sentry-native is writing envelopes, and they introduce new fields without communicating with the mobile team, we won't be adding such fields to the protocol on other layers and the data won't get through to Sentry.
The issue with version (say an app rolls back to an older version of the SDK) is always going to be present though.
The SDK would read that envelope from a newer version and submit a broken version of it.
makes sense, but serialization didn't change much but the endpoint, @denrase take a look at this.
raised #469 thanks @bruno-garcia |
…lope-only-transport-api # Conflicts: # dart/lib/src/protocol/sentry_device.dart # dart/lib/src/protocol/sentry_level.dart # dart/test/protocol/device_test.dart # flutter/test/mocks.mocks.dart
Not sure if I follow. 🤔 Is this something we can do as a test or just something to check manually? Which SDKs are we talking about when its "the latest" and "this version of the SDK"? Should I capture an envelope using the latest native SDK, serialize it and try to deserialize it with the new functionality of this PR and add this as a test? |
yeah sorry I could be more specific, so here we go: Use the Android SDK only, no Flutter, on an emulator, disable the internet so you can inspect the file locally, the important bits is here https://github.com/getsentry/sentry-java/blob/main/sentry-samples/sentry-samples-android/src/main/java/io/sentry/samples/android/MainActivity.java#L33-L52 the envelope would contain an event and an attachment (binary data), now you copy this envelope to the flutter tests and assert the deserialization. now with the deserialized envelope, you capture it again in the Flutter SDK, which writes a file to the disk again, and the Android SDK will send it to sentry, the envelope and image should not be broken, that's how we'd know it works as expected. I guess the deserialization with binary data can be asserted in unit tests, the serialization too, now if the image didn't get broken on sentry, that you can do it manually, we can have a call about that if its the case. |
…lope-only-transport-api
|
@marandaneto Pls take a look at the last commits. Envelopes with arbitrary binary data are now supported like we discussed today. Thank you for your help! 🙇 |
…lope-only-transport-api
…lope-only-transport-api
…lope-only-transport-api # Conflicts: # dart/test/transport/http_transport_test.dart
|
@denrase the |
…lope-only-transport-api
| // We need to replace this when we have the possibility to access envelope deserialization. | ||
| guard let sentrySerialization = NSClassFromString("SentrySerialization") as? NSObject.Type, | ||
| let unmanaged = sentrySerialization.perform(NSSelectorFromString("envelopeWithData:"), with: data), | ||
| let envelope = unmanaged.takeUnretainedValue() as? SentryEnvelope else { | ||
| print("Cannot parse the envelope data") | ||
| result(FlutterError(code: "3", message: "Cannot parse the envelope data", details: nil)) |
There was a problem hiding this comment.
@bruno-garcia @philipphofmann can we expose a way how to deserialize an envelope out of an array of bytes? we're always using a String but now with the possibility of attachments etc this would not work great anymore
📜 Description
TransportAPI to only offersend(envelope: SentryEnvelope)method.SentryEventan associated models.💡 Motivation and Context
SentryEventinstances once they were serialized to an envelope, making it difficult to test transport implementations.💚 How did you test it?
📝 Checklist
🔮 Next steps
#skip-changelog