Remove SentryTransaction from public API #1304

maciejwalkowiak · 2021-03-04T09:28:52Z

📜 Description

Remove SentryTransaction from public API & use separate classes for user facing & protocol operations in the Performance feature.

💡 Motivation and Context

💚 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

maciejwalkowiak · 2021-03-05T09:23:16Z

sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java

@@ -73,6 +73,11 @@ protected void doFilterInternal(
    if (hub.isEnabled()) {
      final String sentryTraceHeader = httpRequest.getHeader(SentryTraceHeader.SENTRY_TRACE_HEADER);

+      hub.configureScope(


ITransaction#setRequest is deprecated now as these kind of properties should be set on the scope and then applied to transaction in SentryClient.

maciejwalkowiak · 2021-03-05T09:29:17Z

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

@@ -424,6 +431,16 @@ public void captureSession(final @NotNull Session session, final @Nullable Objec
    return transaction;
  }

+  private @NotNull SentryTransaction applyScope(


Here very likely we should populate transactions with more data, i just don't want to put everything into single PR.

true, we have an issue for that already

maciejwalkowiak · 2021-03-05T09:31:10Z

sentry/src/main/java/io/sentry/SentryTracer.java

+  @Deprecated
+  @ApiStatus.ScheduledForRemoval
+  public void setRequest(@Nullable Request request) {
+    hub.configureScope(scope -> scope.setRequest(request));


This is not great - ideally we would remove these methods already as it may be surprising to set request on a transaction and realise that it's set on the scope.

I wonder if there are 2 transactions running at the same time and both call setRequest, they are gonna overwrite each other (at least on Android)

We could just have a request field for now and when you do var transation = new Transation you set the fields into that. Same for anything else we need to call into the scope like this.

The deprecation note is there so later we can drop the code on next major.
Would this work?

Yep, done that.

into sentry-tracer

maciejwalkowiak · 2021-03-05T09:40:59Z

Few tests are still todo and likely some polishing in tests, other than that, please take another look @marandaneto @bruno-garcia

codecov-io · 2021-03-05T10:00:46Z

Codecov Report

Merging #1304 (e495ba3) into main (13dd94a) will decrease coverage by 0.18%.
The diff coverage is 85.24%.

@@             Coverage Diff              @@
##               main    #1304      +/-   ##
============================================
- Coverage     75.81%   75.62%   -0.19%     
- Complexity     1796     1830      +34     
============================================
  Files           183      185       +2     
  Lines          6227     6302      +75     
  Branches        622      624       +2     
============================================
+ Hits           4721     4766      +45     
- Misses         1227     1250      +23     
- Partials        279      286       +7

Impacted Files	Coverage Δ	Complexity Δ
...sentry/spring/tracing/SentryTransactionAdvice.java	`77.77% <0.00%> (-10.11%)`	`7.00 <1.00> (-1.00)`
sentry/src/main/java/io/sentry/HubAdapter.java	`8.62% <ø> (ø)`	`4.00 <0.00> (ø)`
sentry/src/main/java/io/sentry/IHub.java	`86.66% <ø> (+6.66%)`	`9.00 <0.00> (+1.00)`
sentry/src/main/java/io/sentry/ISentryClient.java	`89.47% <ø> (ø)`	`10.00 <0.00> (ø)`
sentry/src/main/java/io/sentry/NoOpHub.java	`55.26% <0.00%> (-2.64%)`	`20.00 <0.00> (-1.00)`
...ntry/src/main/java/io/sentry/NoOpSentryClient.java	`60.00% <ø> (ø)`	`6.00 <0.00> (ø)`
sentry/src/main/java/io/sentry/NoOpSpan.java	`31.57% <0.00%> (-1.76%)`	`6.00 <0.00> (ø)`
...entry/src/main/java/io/sentry/NoOpTransaction.java	`21.42% <0.00%> (ø)`	`6.00 <0.00> (ø)`
...entry/src/main/java/io/sentry/SentryBaseEvent.java	`83.33% <ø> (-4.77%)`	`21.00 <0.00> (-1.00)`
...ry/src/main/java/io/sentry/SentryEnvelopeItem.java	`90.47% <ø> (ø)`	`28.00 <0.00> (ø)`
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13dd94a...e495ba3. Read the comment docs.

marandaneto · 2021-03-05T13:19:32Z

sentry/src/main/java/io/sentry/ISpan.java

+   * @return true if transaction has been successfully finished or false if span has been already
+   *     finished
+   */
+  boolean finish();


wondering why we need to return a boolean here

Look into SentryTracer#finish - we need to know if finishing the root span ended up in finishing or not.

Also a bit surprised about returning this.
It's also a breaking change.

Now moved back to void. Technically it's possible that SentryTracer#finish will be invoked by two threads, only one will finish the root span and both will capture the same transaction. As far as I can see transactions are de-duplicated on the Sentry backend side, and it's not something that is likely to happen, so I guess it can stay as it is right now.

But there's no need to return to the user whether it was captured or not due double calling it though so void works fine

sentry/src/main/java/io/sentry/ITransaction.java

bruno-garcia · 2021-03-06T17:01:32Z

sentry/src/main/java/io/sentry/ISpan.java

+   * @return true if transaction has been successfully finished or false if span has been already
+   *     finished
+   */
+  boolean finish();


Also a bit surprised about returning this.
It's also a breaking change.

sentry/src/main/java/io/sentry/ITransaction.java

sentry/src/main/java/io/sentry/NoOpSpan.java

sentry/src/main/java/io/sentry/NoOpTransaction.java

sentry/src/main/java/io/sentry/SentryTracer.java

bruno-garcia · 2021-03-06T17:11:44Z

sentry/src/main/java/io/sentry/SentryTracer.java

+  @Deprecated
+  @ApiStatus.ScheduledForRemoval
+  public void setRequest(@Nullable Request request) {
+    hub.configureScope(scope -> scope.setRequest(request));


We could just have a request field for now and when you do var transation = new Transation you set the fields into that. Same for anything else we need to call into the scope like this.

The deprecation note is there so later we can drop the code on next major.
Would this work?

sentry/src/main/java/io/sentry/SentryTracer.java

bruno-garcia · 2021-03-09T00:55:42Z

@marandaneto good to merge?

sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTransactionAdviceTest.kt

marandaneto · 2021-03-09T13:59:26Z

sentry/src/main/java/io/sentry/SentryTracer.java

+                  if (transaction == this) {
+                    scope.clearTransaction();
+                  }


I guess we are introducing the same bug here, how do we know we are clearing the right transaction? and if its another one? we used to do an equal check, we need to find a way here.

Since we are inside withTransaction, does the lock prevent from setting any transaction during this operation?

ops sorry, I read transaction == null instead of this :D

marandaneto · 2021-03-09T14:26:12Z

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

+        init {
+            val options = SentryOptions()
+            options.dsn = "https://key@sentry.io/proj"
+            hub = spy(Hub(options))


just out of curiosity, what would be the benefit of using spy instead of mock as usual? are we asserting the hub itself? if so, should we not test that thru the HubTest class?

With using mock we can't really test that transaction is cleared from the scope. If we use mock for finish method, we won't really test much.

marandaneto

LGTM
huge win, thanks!

maciejwalkowiak added 6 commits March 3, 2021 16:26

WIP

ab204bd

Merge remote-tracking branch 'origin/main' into sentry-tracer

4202dbb

Refactor Spring modules.

9a364ec

Deprecate methods.

0445c5d

Polish.

8128e8c

Revert sentry-native changes

e25f346

maciejwalkowiak mentioned this pull request Mar 4, 2021

Remove SentryTransaction from public API #1286

Closed

4 tasks

maciejwalkowiak added 5 commits March 4, 2021 11:29

Fix android tests.

41a6ba4

Polish.

89b311b

Apply scope to transaction.

f810653

Fix Span serialization.

319ca38

Create separate protocol class for span.

04ee739

maciejwalkowiak commented Mar 5, 2021

View reviewed changes

Polish.

0f6d9a5

maciejwalkowiak commented Mar 5, 2021

View reviewed changes

maciejwalkowiak added 4 commits March 5, 2021 10:35

Polish.

a053914

Merge branch 'sentry-tracer' of https://github.com/getsentry/sentry-java

43c260c

into sentry-tracer

Merge remote-tracking branch 'origin/main' into sentry-tracer

b0aea8f

Remove transient modifier from Span fields.

9583179

maciejwalkowiak requested review from marandaneto and bruno-garcia March 5, 2021 09:40

maciejwalkowiak added 6 commits March 5, 2021 11:00

Add serialization tests.

e5dcd7b

Add tests.

395e5a2

Deprecate ITransaction#getContexts.

e9b00f1

Changelog.

63f75d6

Fix clearing transaction.

791bdaa

Add test for fixing clearing transaction.

27f80a1

marandaneto reviewed Mar 5, 2021

View reviewed changes

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

Polish

5cdee1e

maciejwalkowiak marked this pull request as ready for review March 5, 2021 14:34

maciejwalkowiak added this to Review in progress in kanban Mar 5, 2021

bruno-garcia mentioned this pull request Mar 6, 2021

Introduce SentryTracer getsentry/sentry-dotnet#868

Closed

bruno-garcia reviewed Mar 6, 2021

View reviewed changes

maciejwalkowiak added 3 commits March 8, 2021 11:59

Put request and contexts on SentryTracer.

389f909

Polish Javadocs.

ed3e6fb

Make finish return void.

eeb8ed5

bruno-garcia approved these changes Mar 9, 2021

View reviewed changes

maciejwalkowiak added 2 commits March 9, 2021 09:01

Remove tag created for keeping transaction name.

1dc5e6e

Spotless.

e495ba3

marandaneto reviewed Mar 9, 2021

View reviewed changes

sentry-spring/src/test/kotlin/io/sentry/spring/tracing/SentryTransactionAdviceTest.kt Outdated Show resolved Hide resolved

marandaneto reviewed Mar 9, 2021

View reviewed changes

Polish.

f2ff5b0

marandaneto approved these changes Mar 10, 2021

View reviewed changes

maciejwalkowiak merged commit 7c3f88a into main Mar 10, 2021

kanban automation moved this from Review in progress to Done Mar 10, 2021

maciejwalkowiak deleted the sentry-tracer branch March 10, 2021 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove SentryTransaction from public API #1304

Remove SentryTransaction from public API #1304

maciejwalkowiak commented Mar 4, 2021 •

edited

maciejwalkowiak Mar 5, 2021

maciejwalkowiak Mar 5, 2021

marandaneto Mar 5, 2021

maciejwalkowiak Mar 5, 2021

marandaneto Mar 5, 2021

bruno-garcia Mar 6, 2021

maciejwalkowiak Mar 8, 2021

maciejwalkowiak commented Mar 5, 2021

codecov-io commented Mar 5, 2021 •

edited

marandaneto Mar 5, 2021

maciejwalkowiak Mar 5, 2021

bruno-garcia Mar 6, 2021

maciejwalkowiak Mar 8, 2021

bruno-garcia Mar 9, 2021

bruno-garcia Mar 6, 2021

bruno-garcia Mar 6, 2021

bruno-garcia commented Mar 9, 2021

marandaneto Mar 9, 2021

maciejwalkowiak Mar 9, 2021

marandaneto Mar 9, 2021

marandaneto Mar 9, 2021

maciejwalkowiak Mar 9, 2021

marandaneto left a comment

Remove SentryTransaction from public API #1304

Remove SentryTransaction from public API #1304

Conversation

maciejwalkowiak commented Mar 4, 2021 • edited

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

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

maciejwalkowiak commented Mar 5, 2021

codecov-io commented Mar 5, 2021 • edited

Codecov Report

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

bruno-garcia commented Mar 9, 2021

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

marandaneto left a comment

Choose a reason for hiding this comment

maciejwalkowiak commented Mar 4, 2021 •

edited

codecov-io commented Mar 5, 2021 •

edited