From 765144c316f042b5297bc0f1bcd7aa8129a89e80 Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 21 Jan 2021 10:51:26 +0100 Subject: [PATCH 1/2] feat: Add addToTransactions to Attachment Add extra parameter addToTransaction to attachment, which specifies if the SDK adds it to transactions or not. The default is false. Fixes GH-1185 --- CHANGELOG.md | 1 + sentry/api/sentry.api | 3 + .../src/main/java/io/sentry/Attachment.java | 64 +++++++++++++++++-- .../src/main/java/io/sentry/SentryClient.java | 17 ++++- .../src/test/java/io/sentry/AttachmentTest.kt | 20 ++++++ .../test/java/io/sentry/SentryClientTest.kt | 15 ++++- 6 files changed, 113 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58fb50cc96..6f4ae384e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ # vNext +* Feat: Add addToTransactions to Attachment (#1191) * Enhancement: Support SENTRY_TRACES_SAMPLE_RATE conf. via env variables (#1171) * Enhancement: Pass request to CustomSamplingContext in Spring integration (#1172) * Ref: Set SpanContext on SentryTransaction to avoid potential NPE (#1173) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index c1509350e0..1fa9a499c5 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -7,12 +7,15 @@ public final class io/sentry/Attachment { public fun (Ljava/lang/String;)V public fun (Ljava/lang/String;Ljava/lang/String;)V public fun (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;)V + public fun (Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;Z)V public fun ([BLjava/lang/String;)V public fun ([BLjava/lang/String;Ljava/lang/String;)V + public fun ([BLjava/lang/String;Ljava/lang/String;Z)V public fun getBytes ()[B public fun getContentType ()Ljava/lang/String; public fun getFilename ()Ljava/lang/String; public fun getPathname ()Ljava/lang/String; + public fun isAddToTransactions ()Z } public final class io/sentry/Breadcrumb : io/sentry/IUnknownPropertiesConsumer, java/lang/Cloneable { diff --git a/sentry/src/main/java/io/sentry/Attachment.java b/sentry/src/main/java/io/sentry/Attachment.java index 6b49afbbd9..ec5d7e1f1a 100644 --- a/sentry/src/main/java/io/sentry/Attachment.java +++ b/sentry/src/main/java/io/sentry/Attachment.java @@ -13,6 +13,7 @@ public final class Attachment { private @Nullable String pathname; private final @NotNull String filename; private final @NotNull String contentType; + private final boolean addToTransactions; /** * We could use Files.probeContentType(path) to determine the content type of the filename. This @@ -24,7 +25,8 @@ public final class Attachment { private static final String DEFAULT_CONTENT_TYPE = "application/octet-stream"; /** - * Initializes an Attachment with bytes and a filename. + * Initializes an Attachment with bytes and a filename. Sets addToTransaction to false + * . * * @param bytes The bytes of file. * @param filename The name of the attachment to display in Sentry. @@ -34,7 +36,8 @@ public Attachment(final @NotNull byte[] bytes, final @NotNull String filename) { } /** - * Initializes an Attachment with bytes, filename and content type. + * Initializes an Attachment with bytes, a filename, and a content type. Sets addToTransaction to + * false. * * @param bytes The bytes of file. * @param filename The name of the attachment to display in Sentry. @@ -44,13 +47,32 @@ public Attachment( final @NotNull byte[] bytes, final @NotNull String filename, final @NotNull String contentType) { + this(bytes, filename, contentType, false); + } + + /** + * Initializes an Attachment with bytes, a filename, a content type, and addToTransactions. + * + * @param bytes The bytes of file. + * @param filename The name of the attachment to display in Sentry. + * @param contentType The content type of the attachment. + * @param addToTransactions true if the SDK should add this attachment to every + * {@link ITransaction} or set to false if it shouldn't. + */ + public Attachment( + final @NotNull byte[] bytes, + final @NotNull String filename, + final @NotNull String contentType, + final boolean addToTransactions) { this.bytes = bytes; this.filename = filename; this.contentType = contentType; + this.addToTransactions = addToTransactions; } /** * Initializes an Attachment with a path. The filename of the file located at the path is used. + * Sets addToTransaction to false. * *

The file located at the pathname is read lazily when the SDK captures an event or * transaction not when the attachment is initialized. The pathname string is converted into an @@ -63,7 +85,8 @@ public Attachment(final @NotNull String pathname) { } /** - * Initializes an Attachment with a path and a filename. + * Initializes an Attachment with a path and a filename. Sets addToTransaction to false + * . * *

The file located at the pathname is read lazily when the SDK captures an event or * transaction not when the attachment is initialized. The pathname string is converted into an @@ -77,7 +100,8 @@ public Attachment(final @NotNull String pathname, final @NotNull String filename } /** - * Initializes an Attachment with a path, a filename and a content type. + * Initializes an Attachment with a path, a filename, and a content type. Sets addToTransaction to + * false. * *

The file located at the pathname is read lazily when the SDK captures an event or * transaction not when the attachment is initialized. The pathname string is converted into an @@ -91,9 +115,31 @@ public Attachment( final @NotNull String pathname, final @NotNull String filename, final @NotNull String contentType) { + this(pathname, filename, contentType, false); + } + + /** + * Initializes an Attachment with a path, a filename, a content type, and addToTransactions. + * + *

The file located at the pathname is read lazily when the SDK captures an event or + * transaction not when the attachment is initialized. The pathname string is converted into an + * abstract pathname before reading the file. + * + * @param pathname The pathname string of the file to upload as an attachment. + * @param filename The name of the attachment to display in Sentry. + * @param contentType The content type of the attachment. + * @param addToTransactions true if the SDK should add this attachment to every + * {@link ITransaction} or set to false if it shouldn't. + */ + public Attachment( + final @NotNull String pathname, + final @NotNull String filename, + final @NotNull String contentType, + final boolean addToTransactions) { this.pathname = pathname; this.filename = filename; this.contentType = contentType; + this.addToTransactions = addToTransactions; } /** @@ -131,4 +177,14 @@ public Attachment( public @NotNull String getContentType() { return contentType; } + + /** + * Returns true if the SDK should add this attachment to every {@link ITransaction} + * and false if it shouldn't. Default is false. + * + * @return true if attachment should be added to every {@link ITransaction}. + */ + public boolean isAddToTransactions() { + return addToTransactions; + } } diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 167b3a95e8..2876a17ca5 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -357,7 +357,7 @@ public void captureSession(final @NotNull Session session, final @Nullable Objec processTransaction((SentryTransaction) transaction); try { final SentryEnvelope envelope = - buildEnvelope(sentryTransaction, getAttachmentsFromScope(scope)); + buildEnvelope(sentryTransaction, filterForTransaction(getAttachmentsFromScope(scope))); if (envelope != null) { transport.send(envelope, hint); } else { @@ -377,6 +377,21 @@ public void captureSession(final @NotNull Session session, final @Nullable Objec return sentryId; } + private List filterForTransaction(List attachments) { + if (attachments == null) { + return null; + } + + List attachmentsToSend = new ArrayList<>(); + for (Attachment attachment : attachments) { + if (attachment.isAddToTransactions()) { + attachmentsToSend.add(attachment); + } + } + + return attachmentsToSend; + } + private @NotNull SentryTransaction processTransaction( final @NotNull SentryTransaction transaction) { if (transaction.getRelease() == null) { diff --git a/sentry/src/test/java/io/sentry/AttachmentTest.kt b/sentry/src/test/java/io/sentry/AttachmentTest.kt index 3929932c24..556b03f8f8 100644 --- a/sentry/src/test/java/io/sentry/AttachmentTest.kt +++ b/sentry/src/test/java/io/sentry/AttachmentTest.kt @@ -2,7 +2,9 @@ package io.sentry import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNull +import kotlin.test.assertTrue class AttachmentTest { @@ -71,4 +73,22 @@ class AttachmentTest { val byteAttachment = Attachment(fixture.bytes, fixture.filename, fixture.contentType) assertEquals(fixture.contentType, byteAttachment.contentType) } + + @Test + fun `default of addToTransactions is false`() { + val fileAttachment = Attachment(fixture.filename) + assertFalse(fileAttachment.isAddToTransactions) + + val byteAttachment = Attachment(fixture.bytes, fixture.filename) + assertFalse(byteAttachment.isAddToTransactions) + } + + @Test + fun `set addToTransactions`() { + val fileAttachment = Attachment(fixture.pathname, fixture.filename, fixture.contentType, true) + assertTrue(fileAttachment.isAddToTransactions) + + val byteAttachment = Attachment(fixture.bytes, fixture.filename, fixture.contentType, true) + assertTrue(byteAttachment.isAddToTransactions) + } } diff --git a/sentry/src/test/java/io/sentry/SentryClientTest.kt b/sentry/src/test/java/io/sentry/SentryClientTest.kt index 7ea5f94209..e343259063 100644 --- a/sentry/src/test/java/io/sentry/SentryClientTest.kt +++ b/sentry/src/test/java/io/sentry/SentryClientTest.kt @@ -69,7 +69,7 @@ class SentryClientTest { whenever(factory.create(any(), any())).thenReturn(transport) } - var attachment = Attachment("hello".toByteArray(), "hello.txt") + var attachment = Attachment("hello".toByteArray(), "hello.txt", "text/plain", true) fun getSut() = SentryClient(sentryOptions) } @@ -763,6 +763,17 @@ class SentryClientTest { verifyAttachmentsInEnvelope(transaction.eventId) } + @Test + fun `when captureTransaction with attachments not added to transaction`() { + val transaction = SentryTransaction("a-transaction") + + val scope = createScopeWithAttachments() + scope.addAttachment(Attachment("hello".toByteArray(), "application/octet-stream")) + fixture.getSut().captureTransaction(transaction, scope, null) + + verifyAttachmentsInEnvelope(transaction.eventId) + } + @Test fun `when scope's active span is a transaction, transaction context is applied to an event`() { val event = SentryEvent() @@ -834,7 +845,7 @@ class SentryClientTest { addAttachment(fixture.attachment) val bytesTooBig = ByteArray((fixture.maxAttachmentSize + 1).toInt()) { 0 } - addAttachment(Attachment(bytesTooBig, "will_get_dropped.txt")) + addAttachment(Attachment(bytesTooBig, "will_get_dropped.txt", "application/octet-stream", true)) } } From 5ac3bb91fda8f958641f4c8a6f608b640c79c67c Mon Sep 17 00:00:00 2001 From: Philipp Hofmann Date: Thu, 21 Jan 2021 11:37:20 +0100 Subject: [PATCH 2/2] Code review --- sentry/api/sentry.api | 1 - sentry/src/main/java/io/sentry/Attachment.java | 2 +- sentry/src/main/java/io/sentry/SentryClient.java | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/sentry/api/sentry.api b/sentry/api/sentry.api index 1fa9a499c5..25162f6432 100644 --- a/sentry/api/sentry.api +++ b/sentry/api/sentry.api @@ -15,7 +15,6 @@ public final class io/sentry/Attachment { public fun getContentType ()Ljava/lang/String; public fun getFilename ()Ljava/lang/String; public fun getPathname ()Ljava/lang/String; - public fun isAddToTransactions ()Z } public final class io/sentry/Breadcrumb : io/sentry/IUnknownPropertiesConsumer, java/lang/Cloneable { diff --git a/sentry/src/main/java/io/sentry/Attachment.java b/sentry/src/main/java/io/sentry/Attachment.java index ec5d7e1f1a..1d291c689d 100644 --- a/sentry/src/main/java/io/sentry/Attachment.java +++ b/sentry/src/main/java/io/sentry/Attachment.java @@ -184,7 +184,7 @@ public Attachment( * * @return true if attachment should be added to every {@link ITransaction}. */ - public boolean isAddToTransactions() { + boolean isAddToTransactions() { return addToTransactions; } } diff --git a/sentry/src/main/java/io/sentry/SentryClient.java b/sentry/src/main/java/io/sentry/SentryClient.java index 2876a17ca5..592ef28798 100644 --- a/sentry/src/main/java/io/sentry/SentryClient.java +++ b/sentry/src/main/java/io/sentry/SentryClient.java @@ -377,7 +377,7 @@ public void captureSession(final @NotNull Session session, final @Nullable Objec return sentryId; } - private List filterForTransaction(List attachments) { + private @Nullable List filterForTransaction(@Nullable List attachments) { if (attachments == null) { return null; }