feat: logcat integration by buenaflor · Pull Request #2620 · getsentry/sentry-java

buenaflor · 2023-03-23T10:38:12Z

📜 Description

Implements a SentryLogcatAdapter that will replace Log calls via bytecode manipulation in the Sentry AGP.

💡 Motivation and Context

Ref: #1620

This adapter is needed by the AGP. See: getsentry/sentry-android-gradle-plugin#455

💚 How did you test it?

Manual testing
Jadx

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

github-actions · 2023-03-23T10:39:17Z

	Messages
📖	Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against 97d1157

github-actions · 2023-03-23T10:55:16Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	341.85 ms	377.19 ms	35.34 ms
Size	1.73 MiB	2.34 MiB	626.85 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`d81684e`	235.73 ms	328.76 ms	93.03 ms

App size

Revision	Plain	With Sentry	Diff
`d81684e`	1.73 MiB	2.26 MiB	547.78 KiB

Previous results on branch: feat/logcat-integration

Startup times

Revision	Plain	With Sentry	Diff
`6dbe27a`	277.26 ms	294.76 ms	17.50 ms
`8a1a524`	366.12 ms	388.69 ms	22.57 ms
`8feb38e`	311.12 ms	376.45 ms	65.32 ms
`3372342`	345.15 ms	379.94 ms	34.79 ms
`174b08b`	363.30 ms	403.38 ms	40.08 ms
`3abf17c`	448.04 ms	479.16 ms	31.12 ms

App size

Revision	Plain	With Sentry	Diff
`6dbe27a`	1.73 MiB	2.26 MiB	548.10 KiB
`8a1a524`	1.73 MiB	2.26 MiB	547.78 KiB
`8feb38e`	1.73 MiB	2.26 MiB	547.63 KiB
`3372342`	1.73 MiB	2.26 MiB	548.10 KiB
`174b08b`	1.73 MiB	2.26 MiB	548.04 KiB
`3abf17c`	1.73 MiB	2.26 MiB	547.63 KiB

codecov · 2023-03-23T14:07:30Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (2ec7569) 81.34% compared to head (97d1157) 81.34%.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #2620   +/-   ##
=========================================
  Coverage     81.34%   81.34%           
  Complexity     4211     4211           
=========================================
  Files           337      337           
  Lines         15575    15575           
  Branches       2034     2034           
=========================================
  Hits          12670    12670           
  Misses         2110     2110           
  Partials        795      795

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

markushi

Looking good, please check my comments

markushi · 2023-03-24T07:02:16Z

+      @NotNull String tag, @NotNull SentryLevel level, String msg, Throwable tr) {
+    Breadcrumb breadcrumb = new Breadcrumb();


We usually like to use the final keyword to indicate that objects will not be re-assigned.

Suggested change

@NotNull String tag, @NotNull SentryLevel level, String msg, Throwable tr) {

Breadcrumb breadcrumb = new Breadcrumb();

@NotNull final String tag, @NotNull final SentryLevel level, final String msg, final Throwable tr) {

final Breadcrumb breadcrumb = new Breadcrumb();

markushi · 2023-03-24T07:05:52Z

+    Sentry.addBreadcrumb(breadcrumb);
+  }
+
+  public static int v(String tag, @NotNull String msg) {


I think it probably makes sense to have tag, msg and throwable marked as @Nullable everywhere

markushi · 2023-03-24T07:07:20Z

+    breadcrumb.setLevel(level);
+    breadcrumb.setData("tag", tag);
+    if (tr != null) {
+      breadcrumb.setData("throwable", tr.getMessage());


I think in the first version it's fine to just have the throwable message inside the breadcrumb, @romtsn what do you think?

yeah, I think that's fine

markushi · 2023-03-24T07:16:23Z

I think it could make sense to add an integration test here as well.

Given an initialized Sentry SDK
When SentryLogcatAdapter.x is called with params y,z
Then a breadcrumb should be captured with y,z

romtsn · 2023-03-24T08:17:37Z

+ * io.sentry.Breadcrumb} for each log. It only replaces log functions that meet a minimum level set
+ * by the user on the Sentry Android Gradle Plugin.
+ */
+@ApiStatus.Internal


Dunno if it makes sense to make it public, people might want to still use it instead of android.util.Log, but without the gradle plugin, what do you think @markushi @buenaflor ?

IMHO I wouldn't 😅 Since the config for minLogLevel lives within the gradle plugin, people calling the SentryLogcatAdapter will always generate breadcrumbs, even for e.g. verbose logging

markushi

LGTM!

markushi · 2023-03-28T13:28:05Z

+import kotlin.test.assertEquals
+
+@RunWith(AndroidJUnit4::class)
+class SentryLogcatAdapterTest {


buenaflor added 2 commits March 23, 2023 10:02

add logcat adapter

10bb83e

add changelog

e923fbe

getsentry-bot and others added 3 commits March 23, 2023 10:40

Format code

52de5a9

fix changelog

dd9787b

merge main

c9d1ce2

buenaflor mentioned this pull request Mar 23, 2023

feat: logcat integration getsentry/sentry-android-gradle-plugin#455

Merged

4 tasks

buenaflor and others added 5 commits March 23, 2023 13:20

add ApiStatus and comment

bc533f2

fix comment

56ccbfe

set SentryLogcatAdapter to final

a82e332

fix build

d1ab0eb

Merge branch 'main' into feat/logcat-integration

b23c660

markushi reviewed Mar 24, 2023

View reviewed changes

romtsn reviewed Mar 24, 2023

View reviewed changes

Comment thread sentry-android-core/src/main/java/io/sentry/android/core/SentryLogcatAdapter.java Outdated

make parameters nullable and use final

9f44057

buenaflor marked this pull request as ready for review March 27, 2023 09:42

buenaflor requested review from adinauer and stefanosiano as code owners March 27, 2023 09:42

buenaflor requested a review from markushi March 27, 2023 10:38

buenaflor and others added 4 commits March 27, 2023 15:32

Merge branch 'main' into feat/logcat-integration

d247b8a

add integration test

ff36acf

Format code

48ccdeb

remove autoinit on SentryLogcatAdapterTest

d502bb7

markushi approved these changes Mar 28, 2023

View reviewed changes

Merge branch 'main' into feat/logcat-integration

97d1157

buenaflor merged commit 17ab223 into main Mar 28, 2023

buenaflor deleted the feat/logcat-integration branch March 28, 2023 15:00

bruno-garcia mentioned this pull request Dec 1, 2023

Add LogCat integration to Android via an EventProcessor getsentry/sentry-dotnet#2926

Merged

		@NotNull String tag, @NotNull SentryLevel level, String msg, Throwable tr) {
		Breadcrumb breadcrumb = new Breadcrumb();

Uh oh!

Uh oh!

Conversation

buenaflor commented Mar 23, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

Uh oh!

github-actions Bot commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: feat/logcat-integration

Startup times

App size

Uh oh!

codecov Bot commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

markushi Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

markushi Mar 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markushi Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

romtsn Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

markushi commented Mar 24, 2023

Uh oh!

romtsn Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

markushi Mar 24, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markushi left a comment

Choose a reason for hiding this comment

Uh oh!

markushi Mar 28, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

github-actions Bot commented Mar 23, 2023 •

edited

Loading

github-actions Bot commented Mar 23, 2023 •

edited

Loading

codecov Bot commented Mar 23, 2023 •

edited

Loading

markushi Mar 24, 2023 •

edited

Loading