Attachments can be manipulated via hints #2046

adinauer · 2022-05-13T06:07:24Z

📜 Description

Attachments can now be manipulated via hints

💡 Motivation and Context

This builds on top of #2045 to enable use cases mentioned in getsentry/sentry-javascript#5036

Closes #1467

💚 How did you test it?

Unit Tests

📝 Checklist

I reviewed the submitted code
I added tests to verify the changes
I updated the docs if needed
No breaking changes

🔮 Next steps

Update docs

…ventProcessor

sentry/src/main/java/io/sentry/hints/Hints.java

adinauer · 2022-05-13T06:20:05Z

sentry/src/main/java/io/sentry/SentryClient.java

@@ -80,6 +80,11 @@ private boolean shouldApplyScopeData(
      hints = new Hints();
    }

+    // TODO what does cached etc mean for devs manipulating hints in beforeSend and eventProcessor?
+    if (shouldApplyScopeData(event, hints)) {


Do we have to prevent manipulation of attachments if !shouldApplyScopeData?

What you did makes sense to only get the attachments from the scope into the hints with this condition.

If we're not applying the scope to the event/hint, an attachment still could show up in the hint because the user added it directly while calling capture. And that's totally fine.

sentry/src/main/java/io/sentry/hints/AttachmentContainer.java

adinauer · 2022-05-13T07:36:26Z

sentry/src/test/java/io/sentry/SentryClientTest.kt

+    }
+
+    @Test
+    fun `passing attachments via hint into breadcrumb ignores them`() {


This test case and the one after it show that attachments in hints have no effect on breadcrumbs. I assume this is what we want. Correct?

h: Why do we have a hint in setBeforeBreadcrumb? I don't get this. This might be frankly confusing. You could do addAttachment in setBeforeBreadcrumb, and nothing would happen.

@marandaneto to answer both I guess 😄

The hint for crumbs are only for filtering, they are not part of the event processing pipeline, I'd say that we don't even need this test.

Is there anything we need to change here? Like passing a different Interface that doesn't have the attachment manipulation methods instead of Hints? Or is it ok if these are basically NOOP for breadcrumbs?

adinauer · 2022-05-13T08:14:09Z

@mattjohnsonpint @timfish fyi

philipphofmann

I like where this is going, but I have a few major items to discuss about the API.

sentry/src/main/java/io/sentry/hints/Hints.java

sentry/src/main/java/io/sentry/hints/AttachmentContainer.java

CHANGELOG.md

philipphofmann · 2022-05-13T11:17:49Z

sentry/src/main/java/io/sentry/hints/Hints.java

+  public static @NotNull Hints withAttachment(@Nullable Attachment attachment) {
+    @NotNull final Hints hints = new Hints();
+    hints.getAttachmentContainer().add(attachment);
+    return hints;
+  }
+
+  public static @NotNull Hints withAttachments(@Nullable List<Attachment> attachments) {
+    @NotNull final Hints hints = new Hints();
+    hints.getAttachmentContainer().addAll(attachments);
+    return hints;
+  }


m: I'm not sure if we really need to add these convenience methods. The user saves one line of code and we have to add two methods for that. I would rather keep it simple.

My intention was to make it as easy as possible to pass an attachment to the captureX() methods.

sentry/src/main/java/io/sentry/hints/Hints.java

Co-authored-by: Philipp Hofmann <[email protected]>

bruno-garcia

The indirection to access the attachments from the Hint IMHO could be dropped in favor of methods hanging straight from the Hints class.

Other than that, LGTM

bruno-garcia · 2022-05-13T13:54:23Z

sentry/src/main/java/io/sentry/SentryClient.java

@@ -80,6 +80,11 @@ private boolean shouldApplyScopeData(
      hints = new Hints();
    }

+    // TODO what does cached etc mean for devs manipulating hints in beforeSend and eventProcessor?
+    if (shouldApplyScopeData(event, hints)) {


What you did makes sense to only get the attachments from the scope into the hints with this condition.

If we're not applying the scope to the event/hint, an attachment still could show up in the hint because the user added it directly while calling capture. And that's totally fine.

sentry/src/main/java/io/sentry/hints/AttachmentContainer.java

sentry/src/main/java/io/sentry/hints/Hints.java

sentry/src/main/java/io/sentry/SentryClient.java

sentry/src/test/java/io/sentry/hints/HintsTest.kt

philipphofmann

I will give this another full pass later today.

sentry/src/main/java/io/sentry/hints/Hints.java

philipphofmann

Only the setBeforeBreadcrumb issue concerns me. Otherwise, I like this. Good job.

philipphofmann · 2022-05-17T16:28:18Z

sentry/src/test/java/io/sentry/SentryClientTest.kt

@@ -1282,7 +1283,7 @@ class SentryClientTest {
    fun `screenshot is added to the envelope from the hint`() {
        val sut = fixture.getSut()
        val attachment = Attachment.fromScreenshot(byteArrayOf())
-        val hints = Hints().also { it.set(SENTRY_SCREENSHOT, attachment) }


The new API is way nicer 👏 .

sentry/src/test/java/io/sentry/SentryClientTest.kt

philipphofmann · 2022-05-17T16:34:23Z

sentry/src/test/java/io/sentry/SentryClientTest.kt

+    }
+
+    @Test
+    fun `passing attachments via hint into breadcrumb ignores them`() {


h: Why do we have a hint in setBeforeBreadcrumb? I don't get this. This might be frankly confusing. You could do addAttachment in setBeforeBreadcrumb, and nothing would happen.

sentry/src/test/java/io/sentry/hints/AttachmentsTest.kt

romtsn · 2022-05-19T13:20:10Z

sentry/src/test/java/io/sentry/SentryClientTest.kt

+    }
+
+    @Test
+    fun `passing attachments via hint into breadcrumb ignores them`() {


@marandaneto to answer both I guess 😄

romtsn · 2022-05-19T14:44:32Z

sentry/src/main/java/io/sentry/hints/Hints.java

-  public void remove(@NotNull String hintType) {
-    internalStorage.remove(hintType);
+  public Hints() {
+    primitiveMappings = new HashMap<>();


can this be done in a static block (and can it be a static map)? Not sure how many instances of HInts we'll have during the lifetime of an app.

romtsn · 2022-05-19T14:45:00Z

sentry/src/main/java/io/sentry/hints/Hints.java

 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;

 public final class Hints {

  private final @NotNull Map<String, Object> internalStorage = new HashMap<String, Object>();
+  private final @NotNull List<Attachment> attachments = new CopyOnWriteArrayList<>();
+  private @Nullable Attachment screenshot = null;


do we need some kind of synchronization around this field (looking at attachments - it's CopyOnWriteArrayList)

Ah no, I don't think it needs to be synchronized, can replace with a different implementation.

…achment-manipulation-via-hints

adinauer · 2022-05-20T05:32:38Z

Merging now to work on some changes in #2045 and not have to deal with merge conflicts. If anything needs to be changed, we can change it in #2045 before merging that.

…EventProcessor as @NotNull Hints object (#2045) * Replace hint Map with Hints object * Add changelog * Fix typo in changelog * Fix link in changelog * Attachments can be manipulated via hints (#2046) * Store attachments in hints and allow manipulation in beforeSend and eventProcessor * Add changelog * Add tests for breadcrumbs and attachments via hints * Update CHANGELOG.md Co-authored-by: Philipp Hofmann <[email protected]> * Rename AttachmentContainer to Attachments * Use long for test * Move attachments into Hints * Fix kotlin/java interop for Hints * Convert screenshot from map entry to property * Rename hint name param * Rename clear to clearAttachments * Use kotlin short version access for getScreenshot * Move AttachmentsTest into HintsTest; add param names * Make primitiveMapping table static * Use ArrayList as there should not be a synchronization issue for hints Co-authored-by: Philipp Hofmann <[email protected]> * Rename Hints to Hint * Remove commented out code * Code Review * Rename SentryFallbackConsumer as it is Hints specific Co-authored-by: Philipp Hofmann <[email protected]>

Store attachments in hints and allow manipulation in beforeSend and e…

0daecc8

…ventProcessor

adinauer requested review from marandaneto and romtsn as code owners May 13, 2022 06:07

adinauer changed the base branch from main to feat/replace-map-with-hints-object May 13, 2022 06:07

Add changelog

26e3bbc

adinauer commented May 13, 2022

View reviewed changes

sentry/src/main/java/io/sentry/hints/Hints.java Show resolved Hide resolved

adinauer commented May 13, 2022

View reviewed changes

sentry/src/main/java/io/sentry/hints/AttachmentContainer.java Outdated Show resolved Hide resolved

Add tests for breadcrumbs and attachments via hints

8fa62bb

adinauer commented May 13, 2022

View reviewed changes

adinauer requested review from philipphofmann and mattjohnsonpint May 13, 2022 08:06

philipphofmann reviewed May 13, 2022

View reviewed changes

Update CHANGELOG.md

f916def

Co-authored-by: Philipp Hofmann <[email protected]>

adinauer changed the title ~~Attachments can now be manipulated via hints~~ Attachments can be manipulated via hints May 13, 2022

bruno-garcia reviewed May 13, 2022

View reviewed changes

Rename AttachmentContainer to Attachments

04edda0

adinauer mentioned this pull request May 13, 2022

Attachments can be manipulated via hints - Extra for attachment methods directly on Hints #2049

Closed

4 tasks

adinauer commented May 16, 2022

View reviewed changes

sentry/src/main/java/io/sentry/SentryClient.java Outdated Show resolved Hide resolved

romtsn reviewed May 16, 2022

View reviewed changes

sentry/src/test/java/io/sentry/hints/HintsTest.kt Outdated Show resolved Hide resolved

adinauer added 5 commits May 16, 2022 19:16

Use long for test

815bf53

Move attachments into Hints

1aa9275

Fix kotlin/java interop for Hints

60dd87a

Convert screenshot from map entry to property

c3d84d9

Rename hint name param

feea24c

philipphofmann reviewed May 17, 2022

View reviewed changes

sentry/src/main/java/io/sentry/hints/Hints.java Show resolved Hide resolved

sentry/src/main/java/io/sentry/hints/Hints.java Show resolved Hide resolved

adinauer added 2 commits May 17, 2022 16:00

Rename clear to clearAttachments

d7d4e72

Use kotlin short version access for getScreenshot

f752f8f

philipphofmann reviewed May 17, 2022

View reviewed changes

adinauer mentioned this pull request May 18, 2022

Hints are now used via a Hints object and passed into beforeSend and EventProcessor as @NotNull Hints object #2045

Merged

4 tasks

Move AttachmentsTest into HintsTest; add param names

22871ce

romtsn approved these changes May 19, 2022

View reviewed changes

adinauer and others added 3 commits May 19, 2022 17:43

Make primitiveMapping table static

34a695e

Use ArrayList as there should not be a synchronization issue for hints

84ab09f

Merge branch 'feat/replace-map-with-hints-object' into feat/allow-att…

357f494

…achment-manipulation-via-hints

adinauer merged commit ac1e895 into feat/replace-map-with-hints-object May 20, 2022

adinauer deleted the feat/allow-attachment-manipulation-via-hints branch May 20, 2022 05:32

adinauer mentioned this pull request Dec 28, 2022

Add the ability to attach on hard crash only #1467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attachments can be manipulated via hints #2046

Attachments can be manipulated via hints #2046

adinauer commented May 13, 2022

adinauer May 13, 2022

bruno-garcia May 13, 2022

adinauer May 13, 2022

philipphofmann May 17, 2022

romtsn May 19, 2022

marandaneto May 19, 2022

adinauer May 19, 2022

adinauer commented May 13, 2022

philipphofmann left a comment

philipphofmann May 13, 2022

adinauer May 13, 2022

bruno-garcia left a comment

bruno-garcia May 13, 2022

philipphofmann left a comment

philipphofmann left a comment

philipphofmann May 17, 2022

philipphofmann May 17, 2022

romtsn May 19, 2022

romtsn May 19, 2022

adinauer May 19, 2022

romtsn May 19, 2022

adinauer May 19, 2022

adinauer May 19, 2022

adinauer commented May 20, 2022

Attachments can be manipulated via hints #2046

Attachments can be manipulated via hints #2046

Conversation

adinauer commented May 13, 2022

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

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

adinauer commented May 13, 2022

philipphofmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruno-garcia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann 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

adinauer commented May 20, 2022