ref: sentry-core changes for console app #473

marandaneto · 2020-06-29T14:34:55Z

📢 Type of change

Bugfix
New feature
Enhancement
Refactoring

📜 Description

ref: sentry-core changes for console app

💡 Motivation and Context

it's not possible to use sentry-core standalone, this is a first PR that makes things a bit easier.

💚 How did you test it?

📝 Checklist

I reviewed submitted code
I added tests to verify changes
All tests passing

🔮 Next steps

bruno-garcia

first pass

sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java

sentry-core/src/main/java/io/sentry/core/Sentry.java

bruno-garcia

It looks like the right direction. Are we far from dropping the draft flag?
It's getting pretty daunting to review this, I hope we can merge soon

sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java

sentry-android-core/src/main/java/io/sentry/android/core/NdkIntegration.java

sentry-core/src/main/java/io/sentry/core/DiagnosticLogger.java

sentry-core/src/main/java/io/sentry/core/SentryOptions.java

sentry-sample/src/main/java/io/sentry/sample/MainActivity.java

marandaneto · 2020-07-09T20:59:30Z

It looks like the right direction. Are we far from dropping the draft flag?
It's getting pretty daunting to review this, I hope we can merge soon

interested to learn why it's getting daunting.
nothing changed from your last review, there are no new commits.
basically the next step is to clean up the code and fix tests, work on your comments if any, nothing more to be changed or added, only removed.

bruno-garcia

This is just a draft and it looks huge:

Probably just 20% of that is actually relevant to the change. Please let's avoid mixing a bunch of refactor in PRs that add a feature or change a core part of the SDK.

sentry-sample/src/main/java/io/sentry/sample/MainActivity.java

sentry-sample/src/main/AndroidManifest.xml

bruno-garcia · 2020-07-10T16:48:13Z

sentry-sample/src/main/res/layout/activity_main.xml

@@ -1,43 +1,56 @@
 <?xml version="1.0" encoding="utf-8"?>
 <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
-              xmlns:tools="http://schemas.android.com/tools"


What changed here? Not sure what to review

probably changed by the IDE automatically and will be reverted, Draft PR

sentry-core/src/main/java/io/sentry/core/SentryOptions.java

sentry-core/src/main/java/io/sentry/core/EnvelopeSender.java

sentry-core/src/main/java/io/sentry/core/AsyncConnectionFactory.java

sentry-android-core/src/main/java/io/sentry/android/core/NdkIntegration.java

sentry-android-core/src/main/java/io/sentry/android/core/EnvelopeFileObserverIntegration.java

sentry-android-core/src/main/java/io/sentry/android/core/AndroidOptionsInitializer.java

marandaneto · 2020-07-11T07:11:32Z

This is just a draft and it looks huge:

Probably just 20% of that is actually relevant to the change. Please let's avoid mixing a bunch of refactor in PRs that add a feature or change a core part of the SDK.

while I agree with the comment,

This is just a draft and it looks huge:

Probably just 20% of that is actually relevant to the change. Please let's avoid mixing a bunch of refactor in PRs that add a feature or change a core part of the SDK.

while I agree with this statement, we agreed to lift a draft PR to share the knowledge of the session data generator (how we did it), this was asked by you btw on a daily standup.

the only code that actually needed to land in the master branch was #474 which is a separate PR, tested, and merged.

all these changes came for free because I actually needed them to make sentry-core to work standalone.
I'm happy to close this PR and make a new one.
This PR is specifically for sending session data to Sentry and validating its APIs, maybe the title of this PR is misleading, but this was stated in the standup.

Let's please not put a lot of useless comments in a Draft PR and focus on the specific bits mentioned in the daily call, which is the changes from sentry-core that actually is gonna be needed for the new sentry-java, all the rest will be discarded, but if I didn't push it, you won't know how I generated the session data.

I'm trying to improve things here, and trying to reuse some work for the future sentry-java, this PR is just early feedback of "how I would fix sentry-core to be usable in sentry-java", so adding negative/useless comments just makes it harder to continue.

happy to discuss offline as well.

bruno-garcia · 2020-07-12T19:29:13Z

while I agree with this statement, we agreed to lift a draft PR to share the knowledge of the session data generator (how we did it), this was asked by you btw on a daily standup.

I wasn't aware they'd come into the same PR. I was genuinely askign why that code is in this PR, since I didn't know if the plan was to merge both things or not, at the same time. Reason why I suggested we don't do that.

I'm trying to improve things here, and trying to reuse some work for the future sentry-java, this PR is just early feedback of "how I would fix sentry-core to be usable in sentry-java", so adding negative/useless comments just makes it harder to continue.

I understand and appreciate the work you put into this and the constant improvement of things. I wasn't trying to add any "negative/useless" comment, simply reiterating what we discussed in several meetings, to try to avoid doing different types of changes (refactor, different features, etc) in a single PR since it can get really difficult to review things.

all these changes came for free because I actually needed them to make sentry-core to work standalone.
I'm happy to close this PR and make a new one.
This PR is specifically for sending session data to Sentry and validating its APIs, maybe the title of this PR is misleading, but this was stated in the standup.

The title is what made it confusing because I thought it was simply the change, as the title says sentry-core changes for console app so all the other code seem not to fit here.

We can catch up offline, no problem. I'm OOO until Tuesday though, sorry.

bruno-garcia · 2020-07-20T13:20:56Z

sentry-android-core/src/main/java/io/sentry/android/core/SentryAndroidOptions.java


+  public SentryAndroidOptions() {
+    setSentryClientName(BuildConfig.SENTRY_CLIENT_NAME + "/" + BuildConfig.VERSION_NAME);
+    setSdkInfo(SdkInfo.createSdkInfo(BuildConfig.SENTRY_CLIENT_NAME, BuildConfig.VERSION_NAME));


This should be an override to SentryOptions ctor which sets it to sentry.java

marandaneto added 9 commits June 29, 2020 16:34

WIP

cf7439c

remove comments

0de2974

wip

e3d6dc2

Merge branch 'master' into feat/console-app

8c7169a

fixes

de1e6e9

fixes

2ae3141

add comments

b09f27b

fix

26ef691

fix

0d97622

marandaneto mentioned this pull request Jul 7, 2020

Hub does not send events #481

Closed

bruno-garcia reviewed Jul 7, 2020

View reviewed changes

sentry-core/src/main/java/io/sentry/core/transport/HttpTransport.java Outdated Show resolved Hide resolved

sentry-core/src/main/java/io/sentry/core/Sentry.java Outdated Show resolved Hide resolved

bruno-garcia mentioned this pull request Jul 9, 2020

Make sure Sentry.init works as intended getsentry/sentry-java#865

Closed

bruno-garcia reviewed Jul 9, 2020

View reviewed changes

bruno-garcia reviewed Jul 10, 2020

View reviewed changes

marandaneto mentioned this pull request Jul 11, 2020

Support for multiple DSNs #485

Closed

marandaneto added 7 commits July 15, 2020 13:44

fix merge conflict

ea162f8

undo not related changes

1e9f72f

fix broken tests

1eb8d95

fix ignored tests

1a34532

addd tests

0eb45f7

improvements

29d53a8

spotless

281099e

marandaneto changed the title ~~fix: sentry-core changes for console app~~ ref: sentry-core changes for console app Jul 16, 2020

marandaneto marked this pull request as ready for review July 16, 2020 11:38

bruno-garcia approved these changes Jul 20, 2020

View reviewed changes

marandaneto added 2 commits July 22, 2020 11:12

fix merge conflicts

ea4d636

move tests

fdfc6e6

fix javadocs

1bf222e

marandaneto merged commit 89b011b into getsentry:master Jul 22, 2020

Uh oh!

ref: sentry-core changes for console app #473

ref: sentry-core changes for console app #473

Uh oh!

Conversation

marandaneto commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📢 Type of change

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Jul 9, 2020

Uh oh!

bruno-garcia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bruno-garcia Jul 10, 2020

Choose a reason for hiding this comment

Uh oh!

marandaneto Jul 11, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marandaneto commented Jul 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bruno-garcia commented Jul 12, 2020

Uh oh!

bruno-garcia Jul 20, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

marandaneto commented Jun 29, 2020 •

edited

Loading

marandaneto commented Jul 11, 2020 •

edited

Loading