Send `source` for transactions #2180

adinauer · 2022-07-21T09:50:30Z

📜 Description

Add transaction source

💡 Motivation and Context

getsentry/team-mobile#30

💚 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

codecov-commenter · 2022-07-21T10:06:50Z

Codecov Report

Merging #2180 (75af364) into main (fc6c136) will decrease coverage by 0.21%.
The diff coverage is 58.45%.

@@             Coverage Diff              @@
##               main    #2180      +/-   ##
============================================
- Coverage     80.87%   80.65%   -0.22%     
- Complexity     3343     3355      +12     
============================================
  Files           236      240       +4     
  Lines         12208    12324     +116     
  Branches       1625     1633       +8     
============================================
+ Hits           9873     9940      +67     
- Misses         1735     1778      +43     
- Partials        600      606       +6

Impacted Files	Coverage Δ
...sentry/spring/tracing/TransactionNameProvider.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/Hub.java	`75.25% <0.00%> (-1.16%)`	⬇️
sentry/src/main/java/io/sentry/HubAdapter.java	`7.93% <0.00%> (+0.12%)`	⬆️
sentry/src/main/java/io/sentry/IHub.java	`88.00% <ø> (+6.51%)`	⬆️
sentry/src/main/java/io/sentry/NoOpHub.java	`46.51% <ø> (+1.05%)`	⬆️
...entry/src/main/java/io/sentry/NoOpTransaction.java	`25.71% <0.00%> (-1.56%)`	⬇️
sentry/src/main/java/io/sentry/Sentry.java	`46.38% <0.00%> (+0.82%)`	⬆️
...ry/src/main/java/io/sentry/TransactionOptions.java	`0.00% <0.00%> (ø)`
sentry/src/main/java/io/sentry/TraceContext.java	`83.43% <40.00%> (-1.42%)`	⬇️
.../main/java/io/sentry/protocol/TransactionInfo.java	`62.85% <62.85%> (ø)`
... and 10 more

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

…nsaction; default to CUSTOM when changing

adinauer · 2022-07-22T05:54:55Z

sentry/api/sentry.api

@@ -376,15 +374,12 @@ public abstract interface class io/sentry/IHub {
 	public fun startTransaction (Lio/sentry/TransactionContext;)Lio/sentry/ITransaction;
 	public fun startTransaction (Lio/sentry/TransactionContext;Lio/sentry/CustomSamplingContext;)Lio/sentry/ITransaction;
 	public abstract fun startTransaction (Lio/sentry/TransactionContext;Lio/sentry/CustomSamplingContext;Z)Lio/sentry/ITransaction;
-	public abstract fun startTransaction (Lio/sentry/TransactionContext;Lio/sentry/CustomSamplingContext;ZLjava/util/Date;)Lio/sentry/ITransaction;


We could consider expanding on this refactoring to further reduce the number of startTransaction overloads for the next major release. Maybe by providing public and internal TransactionOptions.

…entry-java into feat/transaction-name-source

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

marandaneto · 2022-07-25T12:15:47Z

sentry/src/main/java/io/sentry/Hub.java

-      boolean bindToScope,
-      @Nullable Date startTimestamp) {
+      final @NotNull TransactionContext transactionContext,
+      final @NotNull TransactionOptions transactionOptions) {


I noticed that we now have transactionOptions param which is nice, my question is, for all the methods that have changed to accept this new param, are they all @ApiStatus.Internal or are we breaking Public APIs as well?

I only changed the internal ones for now.

sentry/src/main/java/io/sentry/TransactionOptions.java

marandaneto · 2022-07-25T12:19:23Z

sentry/src/main/java/io/sentry/protocol/TransactionNameSource.java

+import org.jetbrains.annotations.NotNull;
+
+@ApiStatus.Internal
+public enum TransactionNameSource implements JsonSerializable {


This is now an enum which is good, what if this SDK ser/deser objects from Hybrid SDKs, they might have sources that this SDK does not know yet, wondering if this should be a String to make it flexible for this use case.
Maybe @bruno-garcia has ideas here.

We could use String in TransactionInfo like this: #2194
Would you prefer that?

Merged the other PRs in here now as seems more stable to me to rely on String based de/serialization

marandaneto · 2022-07-25T12:21:47Z

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

@@ -85,7 +87,7 @@ protected void doFilterInternal(
        // if transaction name is not resolved, the request has not been processed by a controller
        // and we should not report it to Sentry
        if (transactionName != null) {
-          transaction.setName(transactionName);
+          transaction.setName(transactionName, TransactionNameSource.ROUTE);


Better to check with other folks, shouldn't that be URL?

Hmm you're right, it's only ROUTE for SpringMvcTransactionNameProvider but since people might provide their own, it should probably rather be CUSTOM for now. Not sure if we need to improve the TransactionNameProvider interface and add a method with default impl that returns CUSTOM marked as internal which we override and provide what we think works. Not sure I like this approach though.

Created #2188 to show what I mean. Not sure I like the approach though. As an alternative easy way out we could simply set CUSTOM as source instead.

Merged the other PRs in here now as it's still better than sending wrong source

brustolin

LGTM

brustolin · 2022-08-05T09:08:28Z

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

+        final TransactionOptions transactionOptions = new TransactionOptions();
+
+        transactionOptions.setStartTimestamp(appStartTime);
+        transactionOptions.setWaitForChildren(true);
+        transactionOptions.setTransactionFinishedCallback(
+            (finishingTransaction) -> {
+              @Nullable Activity unwrappedActivity = weakActivity.get();
+              if (unwrappedActivity != null) {
+                activityFramesTracker.setMetrics(
+                    unwrappedActivity, finishingTransaction.getEventId());
+              } else {
+                if (options != null) {
+                  options
+                      .getLogger()
+                      .log(
+                          SentryLevel.WARNING,
+                          "Unable to track activity frames as the Activity %s has been destroyed.",
+                          activityName);
+                }
+              }
+            });


l: TransactionOptions in both IF blocks are pretty much the same expect for the 'startTimestamp'. I think we can have a good refactoring here.

Add transaction name source

8170f5b

adinauer requested a review from romtsn as a code owner July 21, 2022 09:50

Remove spy log again

60a01c6

adinauer changed the title ~~Add transaction name source~~ Send source for transactions Jul 21, 2022

Add changelog

d8758a4

adinauer and others added 2 commits July 22, 2022 07:43

Allow setting transaction name source when changing the name of a tra…

e73de49

…nsaction; default to CUSTOM when changing

Format code

e6a8e64

adinauer commented Jul 22, 2022

View reviewed changes

adinauer requested a review from philipphofmann July 22, 2022 06:01

adinauer added 2 commits July 22, 2022 08:03

Commit format change

421f54b

Merge branch 'feat/transaction-name-source' of github.com:getsentry/s…

b1dd2d1

…entry-java into feat/transaction-name-source

marandaneto reviewed Jul 25, 2022

View reviewed changes

sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTransactionAdvice.java Outdated Show resolved Hide resolved

marandaneto reviewed Jul 25, 2022

View reviewed changes

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

marandaneto reviewed Jul 25, 2022

View reviewed changes

adinauer and others added 4 commits July 27, 2022 09:13

CR

5be679e

Merge branch 'main' into feat/transaction-name-source

63045b6

Resolve conflicts

e27d27e

TransactionNameProvider source (#2188)

e89ea1b

adinauer requested a review from marandaneto August 4, 2022 12:10

brustolin approved these changes Aug 5, 2022

View reviewed changes

adinauer and others added 3 commits August 5, 2022 15:44

CR

74c8cb5

Merge branch 'main' into feat/transaction-name-source

62f55a1

Merge branch 'main' into feat/transaction-name-source

75af364

adinauer merged commit 14ec39d into main Aug 5, 2022

adinauer deleted the feat/transaction-name-source branch August 5, 2022 15:47

bruno-garcia mentioned this pull request Aug 5, 2022

Support Transaction info source getsentry/team-mobile#30

Closed

6 tasks

brustolin mentioned this pull request Aug 16, 2022

Add transaction source getsentry/sentry-cocoa#2075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send `source` for transactions #2180

Send `source` for transactions #2180

adinauer commented Jul 21, 2022

codecov-commenter commented Jul 21, 2022 •

edited

adinauer Jul 22, 2022

marandaneto Jul 25, 2022

adinauer Jul 27, 2022

marandaneto Jul 25, 2022

adinauer Aug 2, 2022

adinauer Aug 4, 2022

marandaneto Jul 25, 2022

adinauer Jul 25, 2022

adinauer Jul 27, 2022

adinauer Aug 4, 2022 •

edited

brustolin left a comment

brustolin Aug 5, 2022

Send source for transactions #2180

Send source for transactions #2180

Conversation

adinauer commented Jul 21, 2022

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

codecov-commenter commented Jul 21, 2022 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adinauer Aug 4, 2022 • edited

Choose a reason for hiding this comment

brustolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Send `source` for transactions #2180

Send `source` for transactions #2180

codecov-commenter commented Jul 21, 2022 •

edited

adinauer Aug 4, 2022 •

edited