Refacto : add a Transport class #123
Conversation
dart/lib/src/browser_client.dart
Outdated
| httpClient: options.httpClient, | ||
| clock: options.clock, | ||
| compressPayload: false, | ||
| sdk: Sdk(name: browserSdkName, version: sdkVersion), |
There was a problem hiding this comment.
options.sdkVersion already exists, but its probably null, we'd need to find a way to instantiate it based on the platform (dart, flutter or web), but ideally, we just read this field from the options here
There was a problem hiding this comment.
on a second thought, shouldn't this be just either: sentry.dart or sentry.flutter? Given that this is what the user installed at the end?
There was a problem hiding this comment.
if we need only the identifier of the client, yes, only a String would be fine.
dart/lib/src/browser_client.dart
Outdated
| compressPayload: false, | ||
| sdk: Sdk(name: browserSdkName, version: sdkVersion), | ||
| headersBuilder: SentryClient.buildHeaders, | ||
| platform: platform, |
There was a problem hiding this comment.
wondering if this is the same as options.sentryClientName, could you check it?
There was a problem hiding this comment.
platform is 'dart' or 'javascript' , according to the docs options.sentryClientName is 'sentry.{language}.{platform}/{version}'
There was a problem hiding this comment.
we could do this then?
factory SentryIOClient(SentryOptions options) {
options.platform = sdkPlatform;
SentryIOClient._(options);
}
same for the WebClient, so we move this flag to the options and the transport don't need to take it as a param? we do it automatically.
There was a problem hiding this comment.
✅ platform is now defined at runtime
dart/lib/src/client.dart
Outdated
| /// | ||
| /// Attached to the event payload. | ||
| String get projectId => _dsn.projectId; | ||
| final Transport transport; |
There was a problem hiding this comment.
ideally the transport is read from the options. transport so we don't need to visibleForTesting, and we only instantiate a new transport here if the current optins.transport is a NoOp.
That said, we need a NoOpTransport: https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/transport/NoOpTransport.java
let's ignore the retryAfter for now (https://github.com/getsentry/sentry-dart/projects/1#card-47870442)
There was a problem hiding this comment.
we can remove this, even for testing we can set a mock/fake on the options
dart/lib/src/client.dart
Outdated
| List<EventProcessor> eventProcessors, | ||
| }) { | ||
| for (final processor in eventProcessors) { | ||
| event = processor(event, hint); |
There was a problem hiding this comment.
if one of the processors returned null, means that this is event is dropped, so we should bail out and log it.
eg https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryClient.java#L155-L184
There was a problem hiding this comment.
I can help with those things on a different PR though
dart/lib/src/client.dart
Outdated
| if (scope != null) { | ||
| // Merge the scope transaction. | ||
| if (event.transaction == null) { | ||
| event = event.copyWith(transaction: scope.transaction); | ||
| } | ||
|
|
||
| // Merge the user context. | ||
| if (event.userContext == null) { | ||
| event = event.copyWith(userContext: scope.user); | ||
| } | ||
|
|
||
| // Merge the scope fingerprint. | ||
| if (event.fingerprint == null) { | ||
| event = event.copyWith(fingerprint: scope.fingerprint); | ||
| } | ||
|
|
||
| // Merge the scope breadcrumbs. | ||
| if (event.breadcrumbs == null) { | ||
| event = event.copyWith(breadcrumbs: scope.breadcrumbs); | ||
| } | ||
|
|
||
| // Merge the scope tags. | ||
| if (event.tags == null) { | ||
| event = event.copyWith(tags: scope.tags); | ||
| } | ||
|
|
||
| // Merge the scope extra. | ||
| if (event.extra == null) { | ||
| event = event.copyWith(extra: scope.extra); | ||
| } | ||
|
|
||
| // Merge the scope level. | ||
| if (event.level == null) { | ||
| event = event.copyWith(level: scope.level); | ||
| } | ||
| } |
There was a problem hiding this comment.
for some fields, we actually append items from the Scope to the event, see:
https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/SentryClient.java#L281-L335
dart/lib/src/client.dart
Outdated
| @override | ||
| String toString() => '$SentryClient("${options.dsn}")'; |
There was a problem hiding this comment.
mm not against it, but I don't see how this is useful :)
There was a problem hiding this comment.
I don't know :) until know the postUri was returned... what kind of infos should we return here ? Do we need it ?
There was a problem hiding this comment.
I dont think we need to overwrite the toString for this tbh, makes more sense for data classes only IMO
| final Client httpClient; | ||
|
|
||
| final Dsn _dsn; | ||
|
|
||
| final Sdk sdk; | ||
|
|
||
| final HeadersBuilder headersBuilder; | ||
|
|
||
| final bool compressPayload; | ||
|
|
||
| /// Used by sentry to differentiate browser from io environment | ||
| final String platform; | ||
|
|
||
| final ClockProvider _clock; | ||
|
|
||
| /// Use for browser stacktrace | ||
| final String origin; |
There was a problem hiding this comment.
most of those fields if not all are already in the options, should it not just take options and read from there?
There was a problem hiding this comment.
Updated : the transport is now instantiated from a SentryOptions
| /// The DSN URI. | ||
| @visibleForTesting | ||
| Uri get dsnUri => _dsn.uri; | ||
|
|
||
| /// The Sentry.io public key for the project. | ||
| @visibleForTesting | ||
| // ignore: invalid_use_of_visible_for_testing_member | ||
| String get publicKey => _dsn.publicKey; | ||
|
|
||
| /// The Sentry.io secret key for the project. | ||
| @visibleForTesting | ||
| // ignore: invalid_use_of_visible_for_testing_member | ||
| String get secretKey => _dsn.secretKey; | ||
|
|
||
| /// The ID issued by Sentry.io to your project. | ||
| /// | ||
| /// Attached to the event payload. | ||
| String get projectId => _dsn.projectId; | ||
|
|
||
| String get clientId => sdk.identifier; | ||
|
|
||
| @visibleForTesting | ||
| String get postUri { | ||
| final port = dsnUri.hasPort && | ||
| ((dsnUri.scheme == 'http' && dsnUri.port != 80) || | ||
| (dsnUri.scheme == 'https' && dsnUri.port != 443)) | ||
| ? ':${dsnUri.port}' | ||
| : ''; | ||
| final pathLength = dsnUri.pathSegments.length; | ||
| String apiPath; | ||
| if (pathLength > 1) { | ||
| // some paths would present before the projectID in the dsnUri | ||
| apiPath = | ||
| (dsnUri.pathSegments.sublist(0, pathLength - 1) + ['api']).join('/'); | ||
| } else { | ||
| apiPath = 'api'; | ||
| } | ||
| return '${dsnUri.scheme}://${dsnUri.host}$port/$apiPath/$projectId/store/'; | ||
| } |
There was a problem hiding this comment.
does it make sense to move this to the Dsn class like https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/Dsn.java
ideally, Dsn would offer atributes like dsn.projectId, etc...
There was a problem hiding this comment.
updated : all this logic is now in the Dsnclass
| return '${dsnUri.scheme}://${dsnUri.host}$port/$apiPath/$projectId/store/'; | ||
| } | ||
|
|
||
| Future<SentryId> send(Map<String, dynamic> data) async { |
There was a problem hiding this comment.
our Transport still takes a typed object, SentryEvent in the case, and only here in the transport we'd convert to a json-like object
https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/transport/ITransport.java#L11
| var authHeader = 'Sentry sentry_version=6, sentry_client=$clientId, ' | ||
| 'sentry_timestamp=${now.millisecondsSinceEpoch}, sentry_key=$publicKey'; | ||
| if (secretKey != null) { | ||
| authHeader += ', sentry_secret=$secretKey'; | ||
| } | ||
|
|
||
| mergeAttributes(_getContext(now), into: data); | ||
|
|
||
| final headers = headersBuilder(authHeader); |
There was a problem hiding this comment.
on Java we have an abstraction for this, so we only do it once, and not on every call to send(...), see: https://github.com/getsentry/sentry-java/blob/main/sentry/src/main/java/io/sentry/CredentialsSettingConfigurator.java
could we create such abstraction or at least build it only once on the transport? I think the only moving part here is the sentry_timestamp
There was a problem hiding this comment.
✅ I added a CredentialBuilder
|
|
||
| Future<SentryId> send(Map<String, dynamic> data) async { | ||
| final now = _clock(); | ||
| var authHeader = 'Sentry sentry_version=6, sentry_client=$clientId, ' |
There was a problem hiding this comment.
latest sentry_version is 7, but we'd need to check if there are differences, I've created a separated issue for that
https://github.com/getsentry/sentry-dart/projects/1#card-47871908
- remove projectId from sended data
- remove projectId from sended data - move transport to options
dart/lib/src/client.dart
Outdated
| @@ -64,88 +37,19 @@ abstract class SentryClient { | |||
| /// * https://docs.sentry.io/learn/context/#capturing-the-user | |||
| User userContext; | |||
There was a problem hiding this comment.
this should be part of the SentryClient only as well, it belongs to the Scope.user
dart/lib/src/client.dart
Outdated
| if (response.statusCode != 200) { | ||
| return SentryId.empty(); | ||
| } | ||
| event = event.copyWith(platform: transport.platform); |
There was a problem hiding this comment.
why do we need to read the platform from the transport? its already defining on the SentryEvent ctor based on isWeb.
I believe transport.platform could be done altogether
dart/lib/src/client.dart
Outdated
| @visibleForTesting | ||
| final Transport transport; | ||
| SentryOptions options; |
There was a problem hiding this comment.
it could be private I believe
…port # Conflicts: # dart/lib/src/client.dart # dart/lib/src/sentry_options.dart
…port # Conflicts: # dart/test/sentry_client_test.dart
dart/lib/src/client.dart
Outdated
| if (options.transport is NoOpTransport) { | ||
| options.transport = Transport( | ||
| options: options, | ||
| sdkIdentifier: '${sdkName}/${sdkVersion}', |
There was a problem hiding this comment.
could this be replaced by options.sdkVersion.identifier? it looks duplicated now
There was a problem hiding this comment.
✅ my bad, I thought sdkVersion was just a String, I renamed it sdk, and define it in the ioClient & BrowserClient if null
| }) : eventId = eventId ?? SentryId.newId(), | ||
| platform = platform ?? sdkPlatform, | ||
| timestamp = timestamp ?? getUtcDateTime(); | ||
| platform = platform ?? (isWeb ? browserPlatform : sdkPlatform), |
There was a problem hiding this comment.
if this is going to be set here now, why do we need this on the client/transport? I believe this should be done on the Client, like Java does (via MainEventProcessor)
There was a problem hiding this comment.
✅ I moved it to the client.
dart/lib/src/sentry_options.dart
Outdated
| ClockProvider clock = getUtcDateTime, | ||
| }) { | ||
| }) : _transport = transport ?? NoOpTransport() { |
There was a problem hiding this comment.
why do we have this here? its already NoOpTransport by default, we expose a setter already
There was a problem hiding this comment.
we can't access the setter from the constructor, so if we don't want a null transport we needs this.
There was a problem hiding this comment.
but the field declaration already has NoOpTransport as a value, it should not belong to the ctor, a setter wont be able to set it to null either, because on the setter we need to check if its null and not allow to set it
There was a problem hiding this comment.
we can't write this :
SentryOptions({
this.dsn,
this.environmentAttributes,
this.compressPayload,
this.httpClient,
this.transport,
ClockProvider clock = getUtcDateTime,
})cause this.transport refers to setter and it can't be accessed from here
we can remove the default value in the declaration of the field, but I think : _transport = transport ?? NoOpTransport() is needed
There was a problem hiding this comment.
But we dont need a transport in the ctor, as i said, we dont need any ctor params, options is a mutable class and all fields have setters, you could even remove this ctor altogether
There was a problem hiding this comment.
ah sorry. in french we say : "I understand fast, but you have to explain me long time" :D
| @override | ||
| Dsn get dsn => null; | ||
|
|
||
| @override | ||
| String get origin => null; | ||
|
|
||
| @override | ||
| String get sdkIdentifier => null; |
There was a problem hiding this comment.
this should not be part of the public API, they should only care about the send method
There was a problem hiding this comment.
✅ I just let dsn for now as it is used in a lot of tests
dart/lib/src/version.dart
Outdated
| /// The SDK version reported to Sentry.io in the submitted events. | ||
| const String sdkVersion = '4.0.0'; | ||
|
|
||
| String get sdkName => isWeb ? browserSdkName : ioSdkName; |
There was a problem hiding this comment.
we can make those const private now, if what matters is the sdkName
📢 Type of change
📜 Description
send the event from a dedicated class
💡 Motivation and Context
https://develop.sentry.dev/sdk/unified-api/
💚 How did you test it?
updated existing tests
📝 Checklist
🔮 Next steps