feat(js-sdk): use js transport #2563
Conversation
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/js-sdk-integration #2563 +/- ##
============================================================
+ Coverage 70.98% 87.09% +16.10%
============================================================
Files 17 265 +248
Lines 579 9258 +8679
============================================================
+ Hits 411 8063 +7652
- Misses 168 1195 +1027 ☔ View full report in Codecov by Sentry. |
| @@ -104,6 +104,11 @@ class SentryNative with SentryNativeSafeInvoker implements SentryNativeBinding { | |||
| throw UnsupportedError('$SentryNative.captureEnvelope() is not supported'); | |||
| } | |||
|
|
|||
| @override | |||
| FutureOr<void> captureEnvelopeObject(SentryEnvelope envelope) { | |||
There was a problem hiding this comment.
Is there a particular reason why it's called captureEnvelopeObject instead of just captureEnvelope?
There was a problem hiding this comment.
captureEnvelope already exists which only takes Uint8List and dart sadly doesnt support method overloading, we could theoretically use captureEnvelope but then we would have to convert it back to an envelope object which is unnecessary so that's why I added another captureEnvelope that is supposed to use the actual envelope object and not any other data structure
There was a problem hiding this comment.
other options is to just add another parameter SentryEnvelope envelope to the existing captureEnvelope but imo I'd prefer to have a second function since we dont have union types in dart, maybe in Dart 3 with sealed classes
| // options aren't available until after the options callback executes. | ||
| if (!options.platformChecker.isWeb) { | ||
| options.transport = FileSystemTransport(_native!, options); | ||
| } |
There was a problem hiding this comment.
Would it make sense to set options.transport = JavascriptTransport(_web, options); in the else path here? Then i'd would be in the same place for both.
There was a problem hiding this comment.
it's not possible here since this is happening within SentryFlutter.init where the option is not yet evaluated and we only use the JS SDK if enableSentryJs = true (it's false by default) so the options callback is not evaluated and it will always be false here
I added the comment above that, maybe the comment can be improved to clarify this better
There was a problem hiding this comment.
this will be possible for v9 where we use the JS SDK by default
| tryCatchAsync('captureEnvelopeObject', () async { | ||
| final List<dynamic> envelopeItems = []; | ||
|
|
||
| for (final item in envelope.items) { |
There was a problem hiding this comment.
SentryEnvelopeItemPayload is there because we need the actual json object and not the data, right?
Right now we have some redundancy with dataFactory, maybe we can refactor this so we use the same API both here and in SentryEvent.envelopeStream?
Maybe it also makes sense to make originalObject non-optional?
There was a problem hiding this comment.
The captureEnvelopeObject method does not have attachment size filtering, not sure if we need to include it here or this should be handled by the JS SDK. If we do it here, we don't have to call the JS side with data that will get dropped anyway.
// Only attachments should be filtered according to
// SentryOptions.maxAttachmentSize
if (item.header.type == SentryItemType.attachment &&
data.length > options.maxAttachmentSize) {
continue;
}There was a problem hiding this comment.
SentryEnvelopeItemPayload is there because we need the actual json object and not the data, right?
not necessarily only the json, for example attachments should be bytes.
Right now we have some redundancy with dataFactory, maybe we can refactor this so we use the same API both here and in SentryEvent.envelopeStream?
We could do that, by default the factory methods encode the json into utf-8.
what we could do one of these:
- decode it in captureEnvelope back to json -> dunno about the performance impact
- add or adjust the factory methods to not encode for JS
The captureEnvelopeObject method does not have attachment size filtering
true 👍
There was a problem hiding this comment.
on second look I think it's fine we keep the envelope items encoded in utf-8
📜 Description
Uses js sendEnvelope from the JS SDK transport
💡 Motivation and Context
Part of JS SDK Integration
💚 How did you test it?
Unit test, Integration test, manual
📝 Checklist
sendDefaultPiiis enabled🔮 Next steps
#skip-changelog