Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
a10d740
remove the SentryResponse class
rxlabz Oct 21, 2020
623c904
refactor : add a Transport class
rxlabz Oct 21, 2020
0dcb613
refactor : move the clock initialization to Transport
rxlabz Oct 21, 2020
c97c98b
- refactor : applyScope
rxlabz Oct 21, 2020
477fdac
changelog
rxlabz Oct 21, 2020
b8fb322
fix merge conflicts
rxlabz Oct 22, 2020
fd92e3f
Merge branch 'feature/unified-api' into ref/transport
rxlabz Oct 22, 2020
0944c68
move the headers definition strategy to transport
rxlabz Oct 22, 2020
46829fc
- Instantiate Transport from options
rxlabz Oct 22, 2020
01d4e19
handle event dropping in event processors
rxlabz Oct 22, 2020
fbd001e
fix client._applyScope : merge event and scope tags and extra
rxlabz Oct 22, 2020
4d39531
remove client.toString()
rxlabz Oct 22, 2020
34fb528
fix a test
rxlabz Oct 22, 2020
2437fa4
Merge branch 'feature/unified-api' into ref/transport
rxlabz Oct 22, 2020
57798f5
handle the event serialization in the transport layer
rxlabz Oct 22, 2020
789e154
Merge remote-tracking branch 'origin/feature/unified-api' into ref/tr…
rxlabz Oct 22, 2020
1aa05d6
remove stackFrameFilter references
rxlabz Oct 22, 2020
9895142
Merge remote-tracking branch 'origin/feature/unified-api' into ref/tr…
rxlabz Oct 22, 2020
cb5e309
add a CredentialBuilder
rxlabz Oct 22, 2020
e563d2c
- headers configuration
rxlabz Oct 23, 2020
71b53c8
- fix scope.level precedence
rxlabz Oct 23, 2020
be3f0a2
buildHeaders and fix timestamp test
rxlabz Oct 23, 2020
43b798c
- add a isWeb helper to define the event.platform
rxlabz Oct 23, 2020
19dc91d
feat: apply sample rate
marandaneto Oct 23, 2020
cec1f21
changelog
marandaneto Oct 23, 2020
909d4f9
- add a isWeb helper to define the event.platform and sdkName
rxlabz Oct 23, 2020
8b600dc
- add a isWeb helper to define the event.platform and sdkName
rxlabz Oct 23, 2020
4ff58a6
Merge remote-tracking branch 'origin/feat/sample-rate' into ref/trans…
rxlabz Oct 23, 2020
5493e19
fix
marandaneto Oct 23, 2020
621d6c4
update tests
rxlabz Oct 23, 2020
e11f653
Merge remote-tracking branch 'origin/feat/sample-rate' into ref/trans…
rxlabz Oct 23, 2020
88526e2
remove the platform arg from Client ctor
rxlabz Oct 23, 2020
796b633
set client.options private
rxlabz Oct 23, 2020
fb8b9e0
remove event.platform redefinition
rxlabz Oct 23, 2020
1dbcba4
set options.sdk if null
rxlabz Oct 23, 2020
8b56bce
remove fields from Transport Api
rxlabz Oct 23, 2020
daeb29e
private version consts
rxlabz Oct 23, 2020
098d58c
remove transport from the Client ctor
rxlabz Oct 23, 2020
2833a94
remove options.environmentAttributes
rxlabz Oct 23, 2020
35671d4
event processing : platform
rxlabz Oct 23, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactor : add a Transport class
  • Loading branch information
rxlabz committed Oct 22, 2020
commit 623c9049b0a83ee8b3b9a06ff04408a4faf9cc94
25 changes: 13 additions & 12 deletions dart/lib/src/browser_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
// found in the LICENSE file.

/// A pure Dart client for Sentry.io crash reporting.
import 'dart:convert';
import 'dart:html' show window;

import 'package:http/browser_client.dart';
import 'package:meta/meta.dart';

import 'client.dart';
import 'protocol.dart';
import 'sentry_options.dart';
import 'transport/transport.dart';
import 'version.dart';

SentryClient createSentryClient(SentryOptions options) =>
Expand Down Expand Up @@ -41,19 +42,19 @@ class SentryBrowserClient extends SentryClient {
);
}

SentryBrowserClient._(SentryOptions options, {String origin, String platform})
SentryBrowserClient._(SentryOptions options,
{String origin, @required String platform})
: super.base(
options,
transport: Transport(
dsn: options.dsn,
httpClient: options.httpClient,
clock: options.clock,
compressPayload: false,
sdk: Sdk(name: browserSdkName, version: sdkVersion),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we need only the identifier of the client, yes, only a String would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ fixed

headersBuilder: SentryClient.buildHeaders,
platform: platform,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wondering if this is the same as options.sentryClientName, could you check it?

Copy link
Contributor Author

@rxlabz rxlabz Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

platform is 'dart' or 'javascript' , according to the docs options.sentryClientName is 'sentry.{language}.{platform}/{version}'

Copy link
Contributor

@marandaneto marandaneto Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ platform is now defined at runtime

),
origin: origin,
sdk: Sdk(name: browserSdkName, version: sdkVersion),
platform: platform,
);

@override
List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers,
) =>
// Gzip compression is implicit on browser
utf8.encode(json.encode(data));
}
102 changes: 19 additions & 83 deletions dart/lib/src/client.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import 'dart:async';
import 'dart:convert';

import 'package:meta/meta.dart';
import 'package:sentry/sentry.dart';
Expand All @@ -9,8 +8,8 @@ import 'client_stub.dart'
if (dart.library.io) 'io_client.dart';
import 'protocol.dart';
import 'stack_trace.dart';
import 'transport/transport.dart';
import 'utils.dart';
import 'version.dart';

/// Logs crash reports and events to the Sentry.io service.
abstract class SentryClient {
Expand All @@ -22,36 +21,15 @@ abstract class SentryClient {

SentryClient.base(
this.options, {
String platform,
this.origin,
Sdk sdk,
}) : _dsn = Dsn.parse(options.dsn),
_platform = platform ?? sdkPlatform,
sdk = sdk ?? Sdk(name: sdkName, version: sdkVersion);

final Dsn _dsn;
@required this.transport,
});

@protected
SentryOptions options;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could be private I believe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ fixed


/// 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;
final Transport transport;
Copy link
Contributor

@marandaneto marandaneto Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove this, even for testing we can set a mock/fake on the options

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/// Information about the current user.
///
Expand All @@ -68,51 +46,16 @@ abstract class SentryClient {
/// Use for browser stacktrace
String origin;

/// Used by sentry to differentiate browser from io environment
final String _platform;

final Sdk sdk;

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/';
}

/// Reports an [event] to Sentry.io.
Future<SentryId> captureEvent(
SentryEvent event, {
StackFrameFilter stackFrameFilter,
Scope scope,
}) async {
final now = options.clock();
var authHeader = 'Sentry sentry_version=6, sentry_client=$clientId, '
'sentry_timestamp=${now.millisecondsSinceEpoch}, sentry_key=$publicKey';
if (secretKey != null) {
authHeader += ', sentry_secret=$secretKey';
}

final headers = buildHeaders(authHeader);
event = _processEvent(event, eventProcessors: options.eventProcessors);

final data = <String, dynamic>{
'project': projectId,
'event_id': event.eventId.toString(),
'timestamp': formatDateAsIso8601WithSecondPrecision(event.timestamp),
};

if (options.environmentAttributes != null) {
Expand All @@ -132,22 +75,8 @@ abstract class SentryClient {
),
into: data,
);
mergeAttributes(<String, String>{'platform': _platform}, into: data);

final body = bodyEncoder(data, headers);

final response = await options.httpClient.post(
postUri,
headers: headers,
body: body,
);

if (response.statusCode != 200) {
return SentryId.empty();
}

final eventId = json.decode(response.body)['id'];
return eventId != null ? SentryId.fromId(eventId) : SentryId.empty();
return transport.send(data);
}

/// Reports the [throwable] and optionally its [stackTrace] to Sentry.io.
Expand Down Expand Up @@ -188,15 +117,22 @@ abstract class SentryClient {
options.httpClient?.close();
}

@override
String toString() => '$SentryClient("$postUri")';
SentryEvent _processEvent(
SentryEvent event, {
dynamic hint,
List<EventProcessor> eventProcessors,
}) {
for (final processor in eventProcessors) {
event = processor(event, hint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can help with those things on a different PR though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ fixed

}
return event;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}

@protected
List<int> bodyEncoder(Map<String, dynamic> data, Map<String, String> headers);
@override
String toString() => '$SentryClient("${options.dsn}")';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm not against it, but I don't see how this is useful :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know :) until know the postUri was returned... what kind of infos should we return here ? Do we need it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think we need to overwrite the toString for this tbh, makes more sense for data classes only IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


@protected
@mustCallSuper
Map<String, String> buildHeaders(String authHeader) {
static Map<String, String> buildHeaders(String authHeader) {
final headers = {
'Content-Type': 'application/json',
};
Expand Down
68 changes: 28 additions & 40 deletions dart/lib/src/io_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,60 +2,48 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

/// A pure Dart client for Sentry.io crash reporting.
import 'dart:convert';
import 'dart:io';
import 'package:meta/meta.dart';

/// A pure Dart client for Sentry.io crash reporting.
import 'package:sentry/sentry.dart';
import 'package:sentry/src/transport/transport.dart';

import 'client.dart';
import 'protocol.dart';

SentryClient createSentryClient(SentryOptions options) =>
SentryIOClient(options);

/// Logs crash reports and events to the Sentry.io service.
class SentryIOClient extends SentryClient {
/// Instantiates a client using [dsn] issued to your project by Sentry.io as
/// the endpoint for submitting events.
///
/// [environmentAttributes] contain event attributes that do not change over
/// the course of a program's lifecycle. These attributes will be added to
/// all events captured via this client. The following attributes often fall
/// under this category: [Event.serverName], [Event.release], [Event.environment].
///
/// If [compressPayload] is `true` the outgoing HTTP payloads are compressed
/// using gzip. Otherwise, the payloads are sent in plain UTF8-encoded JSON
/// text. If not specified, the compression is enabled by default.
///
/// If [httpClient] is provided, it is used instead of the default client to
/// make HTTP calls to Sentry.io. This is useful in tests.
factory SentryIOClient(SentryOptions options) => SentryIOClient._(options);

SentryIOClient._(SentryOptions options) : super.base(options);

@override
Map<String, String> buildHeaders(String authHeader) {
final headers = super.buildHeaders(authHeader);
/// Instantiates a client using [SentryOptions]
factory SentryIOClient(SentryOptions options) =>
SentryIOClient._(options, platform: sdkPlatform);

static const sdk = Sdk(name: sdkName, version: sdkVersion);

SentryIOClient._(SentryOptions options, {@required String platform})
: super.base(
options,
transport: Transport(
compressPayload: options.compressPayload,
httpClient: options.httpClient,
clock: options.clock,
sdk: sdk,
dsn: options.dsn,
headersBuilder: buildHeaders,
platform: platform,
),
);

@protected
static Map<String, String> buildHeaders(String authHeader) {
final headers = SentryClient.buildHeaders(authHeader);

// NOTE(lejard_h) overriding user agent on VM and Flutter not sure why
// for web it use browser user agent
headers['User-Agent'] = clientId;
headers['User-Agent'] = sdk.identifier;

return headers;
}

@override
List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers,
) {
// [SentryIOClient] implement gzip compression
// gzip compression is not available on browser
var body = utf8.encode(json.encode(data));
if (options.compressPayload) {
headers['Content-Encoding'] = 'gzip';
body = gzip.encode(body);
}
return body;
}
}
31 changes: 2 additions & 29 deletions dart/lib/src/noop_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import 'client.dart';
import 'protocol.dart';
import 'scope.dart';
import 'sentry_options.dart';
import 'transport/transport.dart';

class NoOpSentryClient implements SentryClient {
NoOpSentryClient._();
Expand All @@ -24,14 +25,7 @@ class NoOpSentryClient implements SentryClient {
String origin;

@override
List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers,
) =>
[];

@override
Map<String, String> buildHeaders(String authHeader) => {};
Transport transport;
Copy link
Contributor

@marandaneto marandaneto Oct 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary? one should not care about the transport when implementing SentryClient, transport comes from options


@override
Future<SentryId> captureEvent(SentryEvent event, {stackFrameFilter, scope}) =>
Expand All @@ -51,29 +45,8 @@ class NoOpSentryClient implements SentryClient {
}) =>
Future.value(SentryId.empty());

@override
String get clientId => 'No-op';

@override
Future<void> close() async {
return;
}

@override
Uri get dsnUri => null;

@override
String get postUri => null;

@override
String get projectId => null;

@override
String get publicKey => null;

@override
Sdk get sdk => null;

@override
String get secretKey => null;
}
17 changes: 17 additions & 0 deletions dart/lib/src/transport/body_encoder.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import 'dart:convert';
import 'dart:io';

List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers, {
bool compressPayload,
}) {
// [SentryIOClient] implement gzip compression
// gzip compression is not available on browser
var body = utf8.encode(json.encode(data));
if (compressPayload) {
headers['Content-Encoding'] = 'gzip';
body = gzip.encode(body);
}
return body;
}
12 changes: 12 additions & 0 deletions dart/lib/src/transport/body_encoder_browser.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import 'dart:convert';

List<int> bodyEncoder(
Map<String, dynamic> data,
Map<String, String> headers, {
bool compressPayload,
}) {
// [SentryIOClient] implement gzip compression
// gzip compression is not available on browser
var body = utf8.encode(json.encode(data));
return body;
}
Loading