Support user context #17

dustin-graham · 2018-05-14T21:49:16Z

allow the client app to specify a user context for logging events and exceptions to sentry.

doesn't work from flutter

googlebot · 2018-05-14T21:49:20Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

dustin-graham · 2018-05-14T21:51:04Z

I signed it!

googlebot · 2018-05-14T21:51:07Z

CLAs look good, thanks!

dustin-graham · 2018-05-14T21:58:19Z

@yjbanov it's failing on the GZIP warning. in local testing with my own app I found that when I upgraded this package to use gzip instead, it failed at runtime in Flutter because it couldn't be found. seems like a Dart version mismatch maybe, but that's why it's failing Travis right now.

dustin-graham · 2018-05-14T21:59:15Z

@zoechi was this what you where looking for here?
https://forum.sentry.io/t/usercontext-without-explicit-support-in-the-client-package/2760

yjbanov

Thanks for the contribution! I most have nitpicks. Design-wise, I'm only curious about making userContext mutable.

yjbanov · 2018-05-14T21:56:34Z

lib/sentry.dart

+  @visibleForTesting
+  User _userContext;
+
+  set userContext(User val) {


We require dartdocs for all public API.

yjbanov · 2018-05-14T21:57:19Z

lib/sentry.dart

@@ -143,6 +143,13 @@ class SentryClient {
  /// Attached to the event payload.
  final String projectId;

+  @visibleForTesting
+  User _userContext;


Why opt for a mutable field rather than a final one, like all others?

yjbanov · 2018-05-14T21:57:47Z

lib/sentry.dart

@@ -330,6 +342,8 @@ class Event {
  /// they must be JSON-serializable.
  final Map<String, dynamic> extra;

+  final User userContext;


yjbanov · 2018-05-14T21:59:22Z

lib/sentry.dart

+///  "email": "[email protected]",
+///  "ip_address": "127.0.0.1",
+///  "subscription": "basic"
+///}


nit1: add one space after the ///
nit2: code snippets in dartdocs must be wrapped in triple-backticks, just like on GitHub.

yjbanov · 2018-05-14T21:59:44Z

lib/sentry.dart

+  final String username;
+  final String email;
+  final String ipAddress;
+  final String subscription;


dartdocs for all public fields. Ideally, link to the docs on sentry.io.

yjbanov · 2018-05-14T21:59:51Z

lib/sentry.dart

+  final String ipAddress;
+  final String subscription;
+
+  const User(this.id, this.username, this.email, this.ipAddress, this.subscription);


Add dartdocs

Use named constructor parameters, like the Event class

I imagine some of these parameters must be non-null. If so, let's add assertions.

none of them are non-null. The docs say that you must have at least an ID OR an IP Address set. but that is up to the client app to enforce which they want. adding an assert to inform the developer during debug that there might be an issue.

yjbanov · 2018-05-14T22:00:03Z

lib/sentry.dart

+
+  const User(this.id, this.username, this.email, this.ipAddress, this.subscription);
+
+  Map<String, String> toJson() {


dustin-graham · 2018-05-14T23:04:48Z

@yjbanov Thanks for the review. I'll push an update tomorrow with those fixes. About the mutable userContext. For my application some of the data for the User is not known until later on at runtime after the user has logged in and done some other things. Allowing the client application to change the user context at any time allows for this flexibility without having to go make a new SentryClient. In one of my commits I had it the other way, where the user passes in the userContext as part of the initial Event environment, but I quickly found that that didn't work for me as the data for the context is only known after login and perhaps further initialization. I did leave in the ability to override the base userContext when sending individual events in case this needs to be overridden for some reason. I could also see removing the userContext from the SentryClient entirely and just make adding in user data an exercise for the user each time they log. But this would likely be burdensome on the client.

yjbanov · 2018-05-15T02:59:15Z

Ok, I think it makes sense. I suppose in some apps you can switch users dynamically too. So let's keep it mutable. Why don't we just make it a mutable field then? The Dart style guide discourages setters without getters.

yjbanov · 2018-05-15T03:01:12Z

Please ping this PR again when you have the changes finalized and I'll look into the GZIP issue. Recently the Dart standard libraries deprecated ALL_CAPS constant names in favor of camelCase.

add dart docs for public API replace subscription property with extras map to better match Sentry documentation

dustin-graham · 2018-05-15T16:48:28Z

@yjbanov I believe I have addressed all the feedback and Travis is now passing.

yjbanov · 2018-05-15T16:58:41Z

test/sentry_test.dart

-            'sentry_timestamp=${fakeClock.now().millisecondsSinceEpoch}, '
+            'sentry_timestamp=${fakeClock
+            .now()
+            .millisecondsSinceEpoch}, '


Wow, did dartfmt do this?

it did. it appears that the travis rule runs dartfmt and fails if there is anything changed after running it. so after I ran dartfmt locally and uploaded it now passes.

I should file a bug.

yjbanov · 2018-05-15T17:00:56Z

Can I merge this? I'll also do a release.

dustin-graham · 2018-05-15T17:11:23Z

@yjbanov yes, let's merge and release! thank you.

yjbanov · 2018-05-15T17:21:10Z

Thank you!

dustin-graham added 6 commits May 14, 2018 12:52

add support for user context

64dcd35

upgrade usage of deprecated GZIP

d94749c

revert gzip usage upgrade

99b20c0

doesn't work from flutter

revert gzip usage upgrade in test

750531a

add tests for user context

f58b55e

fix formatting

1823205

remove print statement

5e022d4

yjbanov reviewed May 14, 2018

View reviewed changes

dustin-graham added 4 commits May 15, 2018 08:29

Make user context a mutable property for SentryClient

2e01023

add dart docs for public API replace subscription property with extras map to better match Sentry documentation

Add additional dart docs and an assertion for User properties

df7ebf8

fix build error

c925605

resolve dartfmt issue

3e568ca

yjbanov reviewed May 15, 2018

View reviewed changes

yjbanov approved these changes May 15, 2018

View reviewed changes

yjbanov merged commit 78ef3f7 into getsentry:master May 15, 2018

zoechi mentioned this pull request May 24, 2018

add support for UserContext in sentry package flutter/flutter#13153

Closed

This was referenced Dec 24, 2021

Error in console when calling Sentry.captureException #680

Closed

Expandos are not allowed on strings, numbers, booleans or null): #681

Closed

pvermaDM mentioned this pull request Jun 24, 2024

Sentry Upgrade in flutter causing other testcases to fail. #2126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support user context #17

Support user context #17

dustin-graham commented May 14, 2018

googlebot commented May 14, 2018

dustin-graham commented May 14, 2018

googlebot commented May 14, 2018

dustin-graham commented May 14, 2018

dustin-graham commented May 14, 2018

yjbanov left a comment

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

yjbanov May 14, 2018

dustin-graham May 15, 2018

dustin-graham commented May 14, 2018

yjbanov commented May 15, 2018

yjbanov commented May 15, 2018

dustin-graham commented May 15, 2018

yjbanov May 15, 2018

dustin-graham May 15, 2018

yjbanov May 15, 2018

yjbanov commented May 15, 2018

dustin-graham commented May 15, 2018

yjbanov commented May 15, 2018


		const User(this.id, this.username, this.email, this.ipAddress, this.subscription);

		Map<String, String> toJson() {

Support user context #17

Support user context #17

Conversation

dustin-graham commented May 14, 2018

googlebot commented May 14, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

dustin-graham commented May 14, 2018

googlebot commented May 14, 2018

dustin-graham commented May 14, 2018

dustin-graham commented May 14, 2018

yjbanov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustin-graham commented May 14, 2018

yjbanov commented May 15, 2018

yjbanov commented May 15, 2018

dustin-graham commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjbanov commented May 15, 2018

dustin-graham commented May 15, 2018

yjbanov commented May 15, 2018