Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add origin to span context #2803

Merged
merged 27 commits into from
Jul 21, 2023
Merged

add origin to span context #2803

merged 27 commits into from
Jul 21, 2023

Conversation

lbloder
Copy link
Collaborator

@lbloder lbloder commented Jun 28, 2023

📜 Description

Add trace origin to SpanContext and set it when automatically creating transactions or spans.

💡 Motivation and Context

Fixes #2692

💚 How did you test it?

📝 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
Copy link
Contributor

github-actions bot commented Jun 28, 2023

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

Generated by 🚫 dangerJS against 10c88ea

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2023

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 309.98 ms 367.33 ms 57.35 ms
Size 1.72 MiB 2.29 MiB 575.54 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
adf8fe3 300.49 ms 357.36 ms 56.87 ms
f60279b 324.60 ms 345.33 ms 20.73 ms
dc67004 273.86 ms 346.37 ms 72.51 ms
d0c08e7 310.28 ms 350.68 ms 40.40 ms
c03a05e 390.40 ms 419.35 ms 28.94 ms
f274c79 313.96 ms 355.96 ms 42.00 ms
f274c79 334.86 ms 348.54 ms 13.68 ms
caf50ec 302.36 ms 325.25 ms 22.89 ms
9246ed4 281.79 ms 352.08 ms 70.29 ms
0310da5 381.20 ms 404.50 ms 23.30 ms

App size

Revision Plain With Sentry Diff
adf8fe3 1.72 MiB 2.29 MiB 575.24 KiB
f60279b 1.72 MiB 2.29 MiB 575.23 KiB
dc67004 1.72 MiB 2.28 MiB 573.45 KiB
d0c08e7 1.72 MiB 2.29 MiB 574.68 KiB
c03a05e 1.72 MiB 2.29 MiB 574.43 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
f274c79 1.72 MiB 2.29 MiB 575.01 KiB
caf50ec 1.72 MiB 2.29 MiB 575.24 KiB
9246ed4 1.72 MiB 2.28 MiB 572.22 KiB
0310da5 1.72 MiB 2.28 MiB 573.45 KiB

Previous results on branch: feat/send_trace_origin

Startup times

Revision Plain With Sentry Diff
d2942c5 286.82 ms 336.78 ms 49.96 ms
a0437fa 319.00 ms 366.47 ms 47.47 ms
05c4514 291.88 ms 358.38 ms 66.50 ms
af877ce 315.78 ms 330.94 ms 15.16 ms
f7d1f90 332.02 ms 377.20 ms 45.18 ms
a67a90a 314.43 ms 376.82 ms 62.39 ms
a8f1f47 261.73 ms 314.85 ms 53.12 ms
38987c2 319.73 ms 437.14 ms 117.41 ms
991f624 328.58 ms 383.94 ms 55.36 ms
aba9d3a 273.60 ms 331.62 ms 58.01 ms

App size

Revision Plain With Sentry Diff
d2942c5 1.72 MiB 2.29 MiB 575.43 KiB
a0437fa 1.72 MiB 2.29 MiB 575.43 KiB
05c4514 1.72 MiB 2.29 MiB 575.18 KiB
af877ce 1.72 MiB 2.29 MiB 575.43 KiB
f7d1f90 1.72 MiB 2.29 MiB 575.43 KiB
a67a90a 1.72 MiB 2.29 MiB 575.43 KiB
a8f1f47 1.72 MiB 2.29 MiB 575.43 KiB
38987c2 1.72 MiB 2.28 MiB 573.75 KiB
991f624 1.72 MiB 2.29 MiB 575.43 KiB
aba9d3a 1.72 MiB 2.29 MiB 575.13 KiB

@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Patch coverage: 81.81% and no project coverage change.

Comparison is base (f60279b) 81.37% compared to head (10c88ea) 81.37%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2803   +/-   ##
=========================================
  Coverage     81.37%   81.37%           
- Complexity     4633     4639    +6     
=========================================
  Files           354      354           
  Lines         17064    17112   +48     
  Branches       2307     2312    +5     
=========================================
+ Hits          13885    13925   +40     
- Misses         2229     2237    +8     
  Partials        950      950           
Impacted Files Coverage Δ
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
...ebflux/SentryWebFilterWithThreadLocalAccessor.java 0.00% <0.00%> (ø)
...racing/SentrySpanClientHttpRequestInterceptor.java 0.00% <0.00%> (ø)
...in/java/io/sentry/internal/gestures/UiElement.java 0.00% <0.00%> (ø)
...y/src/main/java/io/sentry/protocol/SentrySpan.java 84.13% <87.50%> (+0.19%) ⬆️
.../io/sentry/apollo3/SentryApollo3HttpInterceptor.kt 84.27% <100.00%> (+0.06%) ⬆️
...n/java/io/sentry/apollo/SentryApolloInterceptor.kt 82.82% <100.00%> (+0.17%) ⬆️
.../java/io/sentry/graphql/SentryInstrumentation.java 74.24% <100.00%> (+1.22%) ⬆️
...n/java/io/sentry/jdbc/SentryJdbcEventListener.java 88.46% <100.00%> (+0.46%) ⬆️
...in/java/io/sentry/openfeign/SentryFeignClient.java 96.10% <100.00%> (+0.05%) ⬆️
... and 13 more

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

@@ -17,6 +17,7 @@
/** P6Spy JDBC event listener that creates {@link Span}s around database queries. */
@Open
public class SentryJdbcEventListener extends SimpleJdbcEventListener {
public static final String TRACE_ORIGIN = "auto.db.jdbc";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String TRACE_ORIGIN = "auto.db.jdbc";
private static final String TRACE_ORIGIN = "auto.db.jdbc";

@@ -20,6 +20,7 @@
*/
@Open
public class SentrySpanAdvice implements MethodInterceptor {
private static final String TRACE_ORIGIN = "auto.spring_jakarta";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final String TRACE_ORIGIN = "auto.spring_jakarta";
private static final String TRACE_ORIGIN = "auto.function.spring_jakarta.advice";

@@ -26,6 +26,7 @@

@Open
public class SentrySpanClientHttpRequestInterceptor implements ClientHttpRequestInterceptor {
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
private static final String TRACE_ORIGIN = "auto.http.spring_jakarta.resttemplate";

@@ -24,6 +24,7 @@

@Open
public class SentrySpanClientWebRequestFilter implements ExchangeFilterFunction {
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
private static final String TRACE_ORIGIN = "auto.http.spring_jakarta.webclient";

@@ -25,6 +25,7 @@
@ApiStatus.Internal
@Open
public class SentryTransactionAdvice implements MethodInterceptor {
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public static final String TRACE_ORIGIN = "auto.spring_jakarta";
private static final String TRACE_ORIGIN = "auto.function.spring_jakarta.advice";

@@ -34,6 +34,8 @@ import io.sentry.vendor.Base64
import okio.Buffer
import org.jetbrains.annotations.ApiStatus

private const val TRACE_ORIGIN = "auto.apollo3"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const val TRACE_ORIGIN = "auto.apollo3"
private const val TRACE_ORIGIN = "auto.graphql.apollo3"

@@ -27,6 +27,8 @@ import okhttp3.Request
import okhttp3.Response
import java.io.IOException

private const val TRACE_ORIGIN = "auto.okhttp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const val TRACE_ORIGIN = "auto.okhttp"
private const val TRACE_ORIGIN = "auto.http.okhttp"

@@ -21,6 +21,7 @@ import java.util.concurrent.ConcurrentHashMap

private const val PROTOCOL_KEY = "protocol"
private const val ERROR_MESSAGE_KEY = "error_message"
private const val TRACE_ORIGIN = "auto.okhttp"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const val TRACE_ORIGIN = "auto.okhttp"
private const val TRACE_ORIGIN = "auto.http.okhttp"

spy.log Outdated
@@ -0,0 +1,23 @@
1685029529524|0|statement|connection 0|url jdbc:p6spy:hsqldb:mem:testdb|CREATE TABLE person ( id INTEGER IDENTITY PRIMARY KEY, firstName VARCHAR(50) NOT NULL, lastName VARCHAR(50) NOT NULL )|CREATE TABLE person ( id INTEGER IDENTITY PRIMARY KEY, firstName VARCHAR(50) NOT NULL, lastName VARCHAR(50) NOT NULL )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove and gitignore

@adinauer
Copy link
Member

adinauer commented Jul 5, 2023

@stefanosiano @romtsn @markushi can one of you please give the Android parts here a review. Not sure if you'd like more details. Also bytecode manipulation by SAGP may have to be changed as well to include origin.

# Conflicts:
#	CHANGELOG.md
#	sentry-android-core/src/main/java/io/sentry/android/core/ActivityLifecycleIntegration.java
#	sentry-apollo-3/src/main/java/io/sentry/apollo3/SentryApollo3HttpInterceptor.kt
#	sentry-apollo/src/main/java/io/sentry/apollo/SentryApolloInterceptor.kt
#	sentry-spring-jakarta/src/main/java/io/sentry/spring/jakarta/tracing/SentryTracingFilter.java
#	sentry-spring/src/main/java/io/sentry/spring/tracing/SentryTracingFilter.java
#	sentry-spring/src/main/java/io/sentry/spring/webflux/SentryWebFilter.java
@@ -16,6 +16,8 @@ import io.sentry.SpanStatus
import io.sentry.TypeCheckHint.ANDROID_FRAGMENT
import java.util.WeakHashMap

private const val TRACE_ORIGIN = "auto.ui.fragment.lifecycle"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private const val TRACE_ORIGIN = "auto.ui.fragment.lifecycle"
private const val TRACE_ORIGIN = "auto.ui.fragment"

just to align with .activity

return origin;
}

public void setOrigin(@Nullable String origin) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void setOrigin(@Nullable String origin) {
public void setOrigin(final @Nullable String origin) {

@@ -243,6 +244,8 @@ private void startTracing(final @NotNull UiElement target, final @NotNull String
hub.startTransaction(
new TransactionContext(name, TransactionNameSource.COMPONENT, op), transactionOptions);

transaction.getSpanContext().setOrigin(TRACE_ORIGIN + "." + eventType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: I think what I'd like to see instead of auto.ui.gesture_listener.click is rather auto.ui.gesture_listener.old_view_system and auto.ui.gesture_listener.jetpack_compose, to understand where this span is coming from. I think to achieve that you could add a new property origin to UiElement and then set it respectively for Views/JPC and use here.

@@ -140,6 +142,8 @@ class SentryNavigationListener @JvmOverloads constructor(
transactonOptions
)

transaction.spanContext.origin = TRACE_ORIGIN
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: we'd also want to track if it was auto.navigation.jetpack_compose. For that I guess we could just add a new optional ctor parameter and then set it from SentryLifecycleObserver and concat here afterwards

Copy link
Member

@romtsn romtsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the android parts look good to me apart from the comments I left

@lbloder
Copy link
Collaborator Author

lbloder commented Jul 13, 2023

Hey @romtsn and @adinauer,
I implemented your suggestions and added some tests. Would you mind taking another quick look?

@lbloder lbloder marked this pull request as ready for review July 13, 2023 13:52
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

CHANGELOG.md Outdated Show resolved Hide resolved
@romtsn
Copy link
Member

romtsn commented Jul 17, 2023

LGTM too 👍

@lbloder lbloder merged commit 695d3a3 into main Jul 21, 2023
21 checks passed
@lbloder lbloder deleted the feat/send_trace_origin branch July 21, 2023 06:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send trace origin
4 participants