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 : applyScope
+ move origin to Transport
  • Loading branch information
rxlabz committed Oct 22, 2020
commit c97c98b8386d65ba7a70531e45634aa9bb7198ba
2 changes: 1 addition & 1 deletion dart/lib/src/browser_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class SentryBrowserClient extends SentryClient {
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,
),
origin: origin,
);
}
54 changes: 43 additions & 11 deletions dart/lib/src/client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ abstract class SentryClient {

SentryClient.base(
this.options, {
this.origin,
@required this.transport,
});

Expand All @@ -43,9 +42,6 @@ abstract class SentryClient {
/// * https://docs.sentry.io/learn/context/#capturing-the-user
User userContext;
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 be part of the SentryClient only as well, it belongs to the Scope.user

Copy link
Contributor Author

Choose a reason for hiding this comment

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


/// Use for browser stacktrace
String origin;

/// Reports an [event] to Sentry.io.
Future<SentryId> captureEvent(
SentryEvent event, {
Expand All @@ -54,6 +50,8 @@ abstract class SentryClient {
}) async {
event = _processEvent(event, eventProcessors: options.eventProcessors);

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

final data = <String, dynamic>{
'event_id': event.eventId.toString(),
};
Expand All @@ -62,16 +60,10 @@ abstract class SentryClient {
mergeAttributes(options.environmentAttributes.toJson(), into: data);
}

// Merge the user context.
if (userContext != null) {
mergeAttributes(<String, dynamic>{'user': userContext.toJson()},
into: data);
}

mergeAttributes(
event.toJson(
stackFrameFilter: stackFrameFilter,
origin: origin,
origin: transport.origin,
),
into: data,
);
Expand Down Expand Up @@ -128,6 +120,46 @@ abstract class SentryClient {
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.

}

Event _applyScope({@required Event event, @required Scope scope}) {
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

also, missing Contexts, created a card for that https://github.com/getsentry/sentry-dart/projects/1#card-47942441
it doesn't need to be on this PR

}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@override
String toString() => '$SentryClient("${options.dsn}")';

Expand Down
3 changes: 0 additions & 3 deletions dart/lib/src/noop_client.dart
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,6 @@ class NoOpSentryClient implements SentryClient {
@override
SentryOptions options;

@override
String origin;

@override
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


Expand Down
8 changes: 6 additions & 2 deletions dart/lib/src/transport/transport.dart
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,18 @@ class Transport {

final ClockProvider _clock;

/// Use for browser stacktrace
final String origin;

Transport({
@required String dsn,
@required this.compressPayload,
@required this.httpClient,
@required ClockProvider clock,
@required this.sdk,
@required String dsn,
@required ClockProvider clock,
@required this.headersBuilder,
@required this.platform,
this.origin,
}) : _dsn = Dsn.parse(dsn),
_clock = clock ?? getUtcDateTime;

Expand Down
50 changes: 28 additions & 22 deletions dart/test/test_utils.dart
Original file line number Diff line number Diff line change
Expand Up @@ -339,30 +339,32 @@ void runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false}) {
});

const clientUserContext = User(
id: 'client_user',
username: 'username',
email: '[email protected]',
ipAddress: '127.0.0.1');
id: 'client_user',
username: 'username',
email: '[email protected]',
ipAddress: '127.0.0.1',
);
const eventUserContext = User(
id: 'event_user',
username: 'username',
email: '[email protected]',
ipAddress: '127.0.0.1',
extras: <String, String>{'foo': 'bar'});
id: 'event_user',
username: 'username',
email: '[email protected]',
ipAddress: '127.0.0.1',
extras: <String, String>{'foo': 'bar'},
);

final client = SentryClient(
SentryOptions(
dsn: testDsn,
httpClient: httpMock,
clock: fakeClockProvider,
compressPayload: false,
environmentAttributes: SentryEvent(
serverName: 'test.server.com',
release: '1.2.3',
environment: 'staging',
),
final options = SentryOptions(
dsn: testDsn,
httpClient: httpMock,
clock: fakeClockProvider,
compressPayload: false,
environmentAttributes: SentryEvent(
serverName: 'test.server.com',
release: '1.2.3',
environment: 'staging',
),
);

final client = SentryClient(options);
client.userContext = clientUserContext;

try {
Expand All @@ -379,9 +381,13 @@ void runTest({Codec<List<int>, List<int>> gzip, bool isWeb = false}) {
stackTrace: stackTrace,
userContext: eventUserContext,
);
await client.captureEvent(eventWithoutContext);
await client.captureEvent(eventWithoutContext,
scope: Scope(options)..user = clientUserContext);
expect(loggedUserId, clientUserContext.id);
await client.captureEvent(eventWithContext);
await client.captureEvent(
eventWithContext,
scope: Scope(options)..user = clientUserContext,
);
expect(loggedUserId, eventUserContext.id);
}

Expand Down