Add support for Contexts interface #40

wilkomanger · 2019-06-15T19:31:37Z

No description provided.

googlebot · 2019-06-15T19:31:39Z

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.

ℹ️ Googlers: Go here for more info.

wilkomanger · 2019-06-16T10:57:00Z

Signed the CLA!

googlebot · 2019-06-16T10:57:02Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

SammyO · 2019-09-09T16:09:51Z

Any update on this? Would be great to get this merged.

yjbanov

This change also requires tests.

lib/sentry.dart

yjbanov · 2019-09-11T22:27:53Z

lib/sentry.dart

+        break;
+    }
+
+    if (name != null) json['name'] = name;


For consistency let's use { } block syntax everywhere.

Are you sure? I was doing this because this is done in existing code already for serialization. So it would be less consistent with existing code, if you look at Event.toJson for example.

I was looking around line 758. More generally Flutter style guide wants to do this: https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#separate-the-if-expression-from-its-statement. The easiest way to make that work with dartfmt is to use braces.

However, this repo has been pretty lax about style, so I do not insist.

lib/sentry.dart

yjbanov · 2019-09-11T22:30:27Z

Sorry for the delay. If you are still interested in making this change, I'm around to review it. Thanks for the contribution!

wilkomanger · 2019-09-12T09:29:19Z

Cool, will take a look at the suggestions!

yjbanov · 2019-09-25T17:09:01Z

@wilkomanger Are you still interested in merging this PR? The changes LGTM. It just needs some unit tests and it should be good to go.

wilkomanger · 2019-09-25T17:10:12Z

Yes, I will do that next week!

maks · 2019-10-10T04:01:12Z

Would it be possible to allow passing a context into captureException() ?
This would allow reporting the current state of things like battery, freeMemory, etc at the time of error instead of in environmentAttributes which is when the client is created.

maks · 2019-12-18T09:36:25Z

@wilkomanger would you still able to update this PR and add unit tests?
I'm currently using your fork but would prefer to use a published release version of this package for production app.

maks · 2020-01-08T23:27:34Z

@wilkomanger if you don't have time to work on it right now, I can have a go at getting this PR rebased and adding some unit tests for it?

wilkomanger · 2020-01-09T18:16:59Z

@maks That'd be great, thanks! Maybe PR it to my fork and then I can let this PR get approved with the tests?

wilkomanger · 2020-01-09T18:23:41Z

Would it be possible to allow passing a context into captureException() ?
This would allow reporting the current state of things like battery, freeMemory, etc at the time of error instead of in environmentAttributes which is when the client is created.

Maybe you could do that after this PR is merged in a separate one, if it's possible

wilkomanger · 2020-01-09T18:24:23Z

And sorry about the delay everyone, I was/am kinda busy and forgot about it!

maks · 2020-01-09T23:13:32Z

👍 @wilkomanger I'll get to work on that and raise PR against your repo.

maks · 2020-01-10T06:34:08Z

@wilkomanger ok done, I've got a PR against the branch in your repo: wilkomanger#1

that gets it up to date with master from here and adds a test for the json generated (simliar to what events test).

There was alot for code churn here when the web support PR got merged, so the change is actually against the base file now.

Contexts update to upstream, add test, bugfixes

googlebot · 2020-01-10T10:55:48Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

wilkomanger · 2020-01-10T10:56:00Z

@maks Thank you!

maks · 2020-01-10T22:37:58Z

@googlebot I consent.

googlebot · 2020-01-10T22:38:12Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

boottime test independent of test machine timezone

maks · 2020-01-20T23:36:27Z

@yjbanov is there any thing else that is needed to get this merged?

yjbanov · 2020-01-21T17:10:17Z

@yjbanov is there any thing else that is needed to get this merged?

Is this ready for review? LMK and I'll do another pass.

maks · 2020-01-21T20:33:12Z

@yjbanov thanks! Yes please, its ready for another review now.

yjbanov

This looks pretty clean! I only have one small nitpick for the test.

Other than that, let's update CHANGELOG.md, bump the version to 3.0.1, and it's good to go. Thanks!

yjbanov · 2020-01-21T20:47:16Z

test/contexts_test.dart

+      final testApp = App(version: '1.2.3');
+      final testBrowser = Browser(version: '12.3.4');
+
+      final contexts = Contexts(


Let's put this in an Event instance, then call event.toJson() and expect(eventJson['contexts'], ...);. This will also test the logic in Event.toJson().

@yjbanov test improved, changelog updated and version bumped in latest commit.

bruno-garcia

Really nice to see this landing in the Dart sdk

bruno-garcia · 2020-01-22T18:34:12Z

lib/src/base.dart

+              }
+            }
+
+            json[key] = runtime.toJson()..addAll({"type": "runtime"});


Curious if this runtime list approach exists on another Sentry SDK and if yes which one?

I'm not sure, I made it so that in theory you don't have to specify keys or can even specify conflicting ones, from what I remember.

bruno-garcia · 2020-01-22T18:38:49Z

lib/src/base.dart

+    this.freeStorage,
+    this.externalStorageSize,
+    this.externalFreeStorage,
+    this.bootTime,


We recently added some new fields which I Don't think made it to the docs yet. Not at the computer right now but I can check later and make a PR.

bruno-garcia · 2020-01-22T18:39:54Z

lib/src/base.dart

+
+    if (modelId != null) json['model_id'] = modelId;
+
+    if (arch != null) json['arch'] = arch;


We added archs too now for devices with more than one cpu

maks · 2020-01-22T21:40:29Z

Thanks @bruno-garcia,I'd be happy to do those in a followup PR as I'm quite keen to get this one merged first if possible.
@wilkomanger I've made the changes requested by @yjbanov in wilkomanger#3
if you get a chance to merge that into your branch then hopefully this PR is then ready to merge here.

improve test, version bump and changelog

match reported SDK version to version in pubspec

yjbanov · 2020-01-24T00:55:24Z

Awesome! Thank you! Merging and will publish the new version to pub shortly.

dotdoom · 2020-03-29T14:01:09Z

lib/src/base.dart

+  final String screenResolution;
+
+  /// A floating point denoting the screen density.
+  final String screenDensity;


This has to be a double. I get the following error from Sentry:

There was 1 error encountered while processing this event
contexts.device.screen_density: Discarded invalid value

Reason expected a floating point number

Value 2.625

Good point. Just reviewed the docs and it says:

Optional. A floating point denoting the screen density.

I'll review the types and open a PR to fix this

Thanks! Also filed flutter/flutter#53521.

Add support for Contexts interface

6083ec0

Wilko Manger added 7 commits June 16, 2019 12:59

Remove print statement

219ff4c

Apply dartfmt

66e0ca8

Fix batteryLevel assert

3fa1985

Make batteryLevel assert clearer

ff823bb

Remove unused var

8eab7fe

Add null check for runtimes

f08ef81

Fix Browser not being serialized properly

754c294

yrom mentioned this pull request Sep 2, 2019

Add 'transaction' and 'breadcrumbs' in event #43

Merged

SammyO mentioned this pull request Sep 9, 2019

[Sentry] Add captureMessage, and automatically send environmental information for iOS and Android platforms. flutter/flutter#39208

Closed

yjbanov suggested changes Sep 11, 2019

View reviewed changes

Wilko Manger added 7 commits September 12, 2019 16:41

Add trailing commas

d165cdd

Specify types explicitly

1a72e89

Rename Os to OperatingSystem

5206b76

Add link to Sentry documentation

eaac6f5

Add blank line after first paragraph

ef8173b

Fix App documentation

1fa38fa

Add blank line between first and second paragraphs

a0df7b6

maks added 3 commits January 10, 2020 15:41

Merge branch 'master' into contexts

bbdee84

move contexts implementation to new base class

8643fc7

contexts test, fix bug, missing field in device

08e51f3

Merge pull request #1 from maks/contexts

f631cc2

Contexts update to upstream, add test, bugfixes

maks and others added 2 commits January 13, 2020 14:37

boottime test independent of test machine timezone

a124b0b

Merge pull request #2 from maks/context-fix-test

0084603

boottime test independent of test machine timezone

yjbanov suggested changes Jan 21, 2020

View reviewed changes

maks added 2 commits January 22, 2020 10:14

improve contexts test based on PR feedback

1bf165b

bump version to 3.0.1, update Changelog

795975a

bruno-garcia reviewed Jan 22, 2020

View reviewed changes

wilkomanger and others added 3 commits January 23, 2020 20:29

Merge pull request #3 from maks/context-fix-test

56659ee

improve test, version bump and changelog

match reported SDK version to version in pubspec

782143b

Merge pull request #4 from maks/context-fix-test

d7d63d7

match reported SDK version to version in pubspec

yjbanov merged commit 116e4a7 into getsentry:master Jan 24, 2020

dotdoom reviewed Mar 29, 2020

View reviewed changes


		if (modelId != null) json['model_id'] = modelId;

		if (arch != null) json['arch'] = arch;

Uh oh!

Add support for Contexts interface #40

Add support for Contexts interface #40

Uh oh!

Conversation

wilkomanger commented Jun 15, 2019

Uh oh!

googlebot commented Jun 15, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

wilkomanger commented Jun 16, 2019

Uh oh!

googlebot commented Jun 16, 2019

Uh oh!

SammyO commented Sep 9, 2019

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yjbanov commented Sep 11, 2019

Uh oh!

wilkomanger commented Sep 12, 2019

Uh oh!

yjbanov commented Sep 25, 2019

Uh oh!

wilkomanger commented Sep 25, 2019

Uh oh!

maks commented Oct 10, 2019

Uh oh!

maks commented Dec 18, 2019

Uh oh!

maks commented Jan 8, 2020

Uh oh!

wilkomanger commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilkomanger commented Jan 9, 2020

Uh oh!

wilkomanger commented Jan 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maks commented Jan 9, 2020

Uh oh!

maks commented Jan 10, 2020

Uh oh!

googlebot commented Jan 10, 2020

Uh oh!

wilkomanger commented Jan 10, 2020

Uh oh!

maks commented Jan 10, 2020

Uh oh!

googlebot commented Jan 10, 2020

Uh oh!

maks commented Jan 20, 2020

Uh oh!

yjbanov commented Jan 21, 2020

Uh oh!

maks commented Jan 21, 2020

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilkomanger commented Jan 9, 2020 •

edited

Loading

wilkomanger commented Jan 9, 2020 •

edited

Loading