-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fix not eligible for auto proxying warnings #3154
Conversation
…re to fix the not eligible for auto-proxying warnings caused and remove hub dependency
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
937879e | 417.64 ms | 550.45 ms | 132.81 ms |
8fd337b | 349.16 ms | 459.22 ms | 110.06 ms |
eecfab6 | 399.27 ms | 461.82 ms | 62.55 ms |
9119d59 | 407.12 ms | 509.64 ms | 102.52 ms |
0bd723b | 395.44 ms | 472.66 ms | 77.22 ms |
4e260b3 | 378.73 ms | 454.18 ms | 75.45 ms |
93a76ca | 377.96 ms | 447.52 ms | 69.56 ms |
1f3652d | 361.62 ms | 424.76 ms | 63.14 ms |
7eccfdb | 366.98 ms | 440.27 ms | 73.29 ms |
baaf637 | 375.71 ms | 441.58 ms | 65.87 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
937879e | 1.72 MiB | 2.27 MiB | 558.42 KiB |
8fd337b | 1.72 MiB | 2.27 MiB | 555.00 KiB |
eecfab6 | 1.72 MiB | 2.27 MiB | 558.56 KiB |
9119d59 | 1.70 MiB | 2.27 MiB | 583.84 KiB |
0bd723b | 1.72 MiB | 2.29 MiB | 578.09 KiB |
4e260b3 | 1.72 MiB | 2.27 MiB | 554.95 KiB |
93a76ca | 1.72 MiB | 2.29 MiB | 576.75 KiB |
1f3652d | 1.72 MiB | 2.27 MiB | 554.95 KiB |
7eccfdb | 1.72 MiB | 2.27 MiB | 556.93 KiB |
baaf637 | 1.72 MiB | 2.27 MiB | 558.42 KiB |
Previous results on branch: feat/fix-not-eligible-for-auto-proxying
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b2afcef | 358.50 ms | 428.94 ms | 70.44 ms |
8dfa3cb | 448.15 ms | 537.58 ms | 89.43 ms |
c4cd472 | 373.69 ms | 445.10 ms | 71.41 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
b2afcef | 1.70 MiB | 2.27 MiB | 583.82 KiB |
8dfa3cb | 1.70 MiB | 2.27 MiB | 583.82 KiB |
c4cd472 | 1.70 MiB | 2.27 MiB | 583.82 KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I restored the Advice
constructors as this would be a breaking change since not all of those are marked internal.
@@ -95,13 +95,13 @@ public abstract interface annotation class io/sentry/spring/jakarta/checkin/Sent | |||
} | |||
|
|||
public class io/sentry/spring/jakarta/checkin/SentryCheckInAdvice : org/aopalliance/intercept/MethodInterceptor { | |||
public fun <init> (Lio/sentry/IHub;)V |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this part yesterday but the Advice
classes are mostly public and not marked internal so we should keep the old constructor in place to not cause a breaking change.
@@ -190,9 +190,9 @@ public final class io/sentry/spring/jakarta/graphql/SentrySpringSubscriptionHand | |||
|
|||
public class io/sentry/spring/jakarta/tracing/SentryAdviceConfiguration { | |||
public fun <init> ()V | |||
public fun sentrySpanAdvice (Lio/sentry/IHub;)Lorg/aopalliance/aop/Advice; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't consider this a breaking change and I'm not sure there's a way around this.
Tests still need some love. |
Looks good on my end now. Thanks! |
@adase11 thanks for the feedback. |
📜 Description
Fix not eligible for auto proxying warnings by:
Advice
Beans@Configuration
or@Bean
with@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
💡 Motivation and Context
Fixes #3042
💚 How did you test it?
Manually by checking the log output
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps