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
12 changes: 1 addition & 11 deletions dart/lib/src/browser_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import 'package:meta/meta.dart';

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

SentryClient createSentryClient(SentryOptions options) =>
Expand All @@ -32,7 +31,6 @@ class SentryBrowserClient extends SentryClient {
options.httpClient ??= BrowserClient();

// origin is necessary for sentry to resolve stacktrace
origin ??= '${window.location.origin}/';

return SentryBrowserClient._(
options,
Expand All @@ -45,13 +43,5 @@ class SentryBrowserClient extends SentryClient {
SentryOptions options, {
String origin,
@required String platform,
}) : super.base(
options,
transport: Transport(
options: options,
sdkIdentifier: '${browserSdkName}/${sdkVersion}',
platform: platform,
origin: origin,
),
);
}) : super.base(options, origin: '${window.location.origin}/');
}
20 changes: 13 additions & 7 deletions dart/lib/src/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ import 'dart:async';

import 'package:meta/meta.dart';
import 'package:sentry/sentry.dart';
import 'package:sentry/src/transport/noop_transport.dart';

import 'client_stub.dart'
if (dart.library.html) 'browser_client.dart'
if (dart.library.io) 'io_client.dart';
import 'protocol.dart';
import 'transport/transport.dart';

/// Logs crash reports and events to the Sentry.io service.
abstract class SentryClient {
Expand All @@ -17,13 +17,19 @@ abstract class SentryClient {
/// `dart:html` is available, otherwise it will throw an unsupported error.
factory SentryClient(SentryOptions options) => createSentryClient(options);

SentryClient.base(this.options, {@required this.transport});
SentryClient.base(this.options, {String origin}) {
if (options.transport is NoOpTransport) {
options.transport = Transport(
options: options,
sdkIdentifier: '${sdkName}/${sdkVersion}',
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be replaced by options.sdkVersion.identifier? it looks duplicated now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ my bad, I thought sdkVersion was just a String, I renamed it sdk, and define it in the ioClient & BrowserClient if null

origin: origin,
);
}
}

@protected
SentryOptions options;

@visibleForTesting
final Transport transport;
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


/// Information about the current user.
///
Expand All @@ -47,9 +53,9 @@ abstract class SentryClient {

event = _applyScope(event: event, scope: scope);

event = event.copyWith(platform: transport.platform);
event = event.copyWith(platform: sdkPlatform);

return transport.send(event);
return options.transport.send(event);
}

/// Reports the [throwable] and optionally its [stackTrace] to Sentry.io.
Expand Down
14 changes: 3 additions & 11 deletions dart/lib/src/io_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,9 @@
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 'sentry_options.dart';
import 'version.dart';

SentryClient createSentryClient(SentryOptions options) =>
SentryIOClient(options);
Expand All @@ -20,12 +19,5 @@ class SentryIOClient extends SentryClient {
SentryIOClient._(options, platform: sdkPlatform);

SentryIOClient._(SentryOptions options, {@required String platform})
: super.base(
options,
transport: Transport(
options: options,
sdkIdentifier: '${sdkName}/${sdkVersion}',
platform: platform,
),
);
: super.base(options);
}
4 changes: 0 additions & 4 deletions dart/lib/src/noop_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import 'client.dart';
import 'protocol.dart';
import 'scope.dart';
import 'sentry_options.dart';
import 'transport/transport.dart';

class NoOpSentryClient implements SentryClient {
NoOpSentryClient._();
Expand All @@ -21,9 +20,6 @@ class NoOpSentryClient implements SentryClient {
@override
SentryOptions options;

@override
Transport transport;

@override
Future<SentryId> captureEvent(
SentryEvent event, {
Expand Down
14 changes: 12 additions & 2 deletions dart/lib/src/sentry_options.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import 'package:http/http.dart';
import 'package:sentry/sentry.dart';
import 'package:sentry/src/transport/noop_transport.dart';

import 'diagnostic_logger.dart';
import 'hub.dart';
import 'protocol.dart';
Expand Down Expand Up @@ -118,7 +120,14 @@ class SentryOptions {

List<String> get inAppIncludes => List.unmodifiable(_inAppIncludes);

// TODO: transport, transportGate, connectionTimeoutMillis, readTimeoutMillis, hostnameVerifier, sslSocketFactory, proxy
Transport _transport = NoOpTransport();

Transport get transport => _transport;

set transport(Transport transport) =>
_transport = transport ?? NoOpTransport();

// TODO: transportGate, connectionTimeoutMillis, readTimeoutMillis, hostnameVerifier, sslSocketFactory, proxy

/// Sets the distribution. Think about it together with release and environment
String dist;
Expand All @@ -139,8 +148,9 @@ class SentryOptions {
this.environmentAttributes,
this.compressPayload,
this.httpClient,
Transport transport,
ClockProvider clock = getUtcDateTime,
}) {
}) : _transport = transport ?? NoOpTransport() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we have this here? its already NoOpTransport by default, we expose a setter already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can't access the setter from the constructor, so if we don't want a null transport we needs this.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah sorry. in french we say : "I understand fast, but you have to explain me long time" :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✅ removed !

_clock = clock;
}

Expand Down
17 changes: 17 additions & 0 deletions dart/lib/src/transport/noop_transport.dart
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import 'dart:async';

import 'package:sentry/sentry.dart';

class NoOpTransport implements Transport {
@override
Dsn get dsn => null;

@override
String get origin => null;

@override
String get sdkIdentifier => null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this should not be part of the public API, they should only care about the send method

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.

✅ I just let dsn for now as it is used in a lot of tests


@override
Future<SentryId> send(SentryEvent event) => Future.value(SentryId.empty());
}
4 changes: 0 additions & 4 deletions dart/lib/src/transport/transport.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,6 @@ class Transport {
/// Use for browser stacktrace
final String origin;

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

final String sdkIdentifier;

CredentialBuilder _credentialBuilder;
Expand All @@ -36,7 +33,6 @@ class Transport {
Transport({
@required SentryOptions options,
@required this.sdkIdentifier,
@required this.platform,
this.origin,
}) : _options = options,
dsn = Dsn.parse(options.dsn),
Expand Down
7 changes: 6 additions & 1 deletion dart/lib/src/version.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,12 @@ const String browserSdkName = 'sentry.dart.browser';
/// The name of the SDK platform reported to Sentry.io in the submitted events.
///
/// Used for IO version.
const String sdkPlatform = 'dart';
String get sdkPlatform => isWeb ? browserPlatform : ioSdkPlatform;

/// The name of the SDK platform reported to Sentry.io in the submitted events.
///
/// Used for IO version.
const String ioSdkPlatform = 'dart';

/// Used to report browser Stacktrace to sentry.
const String browserPlatform = 'javascript';
46 changes: 23 additions & 23 deletions dart/test/test_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,14 @@ Future testCaptureException(
expect('$sentryId', 'testeventid');
}

expect(postUri, client.transport.dsn.postUri);
expect(postUri, client.options.transport.dsn.postUri);

testHeaders(
headers,
fakeClockProvider,
compressPayload: compressPayload,
withUserAgent: !isWeb,
sdkName: isWeb ? browserSdkName : sdkName,
sdkName: sdkName,
);

Map<String, dynamic> data;
Expand Down Expand Up @@ -182,48 +182,48 @@ Future testCaptureException(
void runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false}) {
test('can parse DSN', () async {
final client = SentryClient(SentryOptions(dsn: testDsn));
expect(client.transport.dsn.uri, Uri.parse(testDsn));
expect(client.transport.dsn.postUri,
expect(client.options.transport.dsn.uri, Uri.parse(testDsn));
expect(client.options.transport.dsn.postUri,
'https://sentry.example.com/api/1/store/');
expect(client.transport.dsn.publicKey, 'public');
expect(client.transport.dsn.secretKey, 'secret');
expect(client.transport.dsn.projectId, '1');
expect(client.options.transport.dsn.publicKey, 'public');
expect(client.options.transport.dsn.secretKey, 'secret');
expect(client.options.transport.dsn.projectId, '1');
await client.close();
});

test('can parse DSN without secret', () async {
final client = SentryClient(SentryOptions(dsn: _testDsnWithoutSecret));
expect(client.transport.dsn.uri, Uri.parse(_testDsnWithoutSecret));
expect(client.transport.dsn.postUri,
expect(client.options.transport.dsn.uri, Uri.parse(_testDsnWithoutSecret));
expect(client.options.transport.dsn.postUri,
'https://sentry.example.com/api/1/store/');
expect(client.transport.dsn.publicKey, 'public');
expect(client.transport.dsn.secretKey, null);
expect(client.transport.dsn.projectId, '1');
expect(client.options.transport.dsn.publicKey, 'public');
expect(client.options.transport.dsn.secretKey, null);
expect(client.options.transport.dsn.projectId, '1');
await client.close();
});

test('can parse DSN with path', () async {
final client = SentryClient(SentryOptions(dsn: _testDsnWithPath));
expect(client.transport.dsn.uri, Uri.parse(_testDsnWithPath));
expect(client.options.transport.dsn.uri, Uri.parse(_testDsnWithPath));
expect(
client.transport.dsn.postUri,
client.options.transport.dsn.postUri,
'https://sentry.example.com/path/api/1/store/',
);
expect(client.transport.dsn.publicKey, 'public');
expect(client.transport.dsn.secretKey, 'secret');
expect(client.transport.dsn.projectId, '1');
expect(client.options.transport.dsn.publicKey, 'public');
expect(client.options.transport.dsn.secretKey, 'secret');
expect(client.options.transport.dsn.projectId, '1');
await client.close();
});
test('can parse DSN with port', () async {
final client = SentryClient(SentryOptions(dsn: _testDsnWithPort));
expect(client.transport.dsn.uri, Uri.parse(_testDsnWithPort));
expect(client.options.transport.dsn.uri, Uri.parse(_testDsnWithPort));
expect(
client.transport.dsn.postUri,
client.options.transport.dsn.postUri,
'https://sentry.example.com:8888/api/1/store/',
);
expect(client.transport.dsn.publicKey, 'public');
expect(client.transport.dsn.secretKey, 'secret');
expect(client.transport.dsn.projectId, '1');
expect(client.options.transport.dsn.publicKey, 'public');
expect(client.options.transport.dsn.secretKey, 'secret');
expect(client.options.transport.dsn.projectId, '1');
await client.close();
});
test('sends client auth header without secret', () async {
Expand Down Expand Up @@ -268,7 +268,7 @@ void runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false}) {
withUserAgent: !isWeb,
compressPayload: false,
withSecret: false,
sdkName: isWeb ? browserSdkName : sdkName,
sdkName: sdkName,
);

await client.close();
Expand Down