-
Notifications
You must be signed in to change notification settings - Fork 846
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 flaky spring scheduling tests #12469
Conversation
@@ -39,7 +39,7 @@ | |||
*/ | |||
@Aspect | |||
final class SpringSchedulingInstrumentationAspect { | |||
public static final String INSTRUMENTATION_NAME = "io.opentelemetry.spring-scheduling-3.1"; | |||
private static final String INSTRUMENTATION_NAME = "io.opentelemetry.spring-boot-autoconfigure"; |
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.
Instrumentation name should align with the module that contains the instrumentation code. It would probably be better to move this code elsewhere. cc @zeitlinger @jeanbisutti
OpenTelemetry.class, | ||
R2DbcInstrumentationAutoConfigurationTest::openTelemetry); | ||
|
||
private static OpenTelemetry openTelemetry() { |
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.
an alternative would be to move this wrapping to the LibraryTestRunner
@trask WDYT?
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.
moving it makes sense to me, since LibraryTestRunner
manages the SDK lifecycle
OpenTelemetry.class, | ||
R2DbcInstrumentationAutoConfigurationTest::openTelemetry); | ||
|
||
private static OpenTelemetry openTelemetry() { |
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.
moving it makes sense to me, since LibraryTestRunner
manages the SDK lifecycle
@@ -91,10 +97,8 @@ private LibraryTestRunner() { | |||
@Override | |||
public void beforeTestClass() { | |||
// just in case: if there was any test that modified the global instance, reset it | |||
if (GlobalOpenTelemetry.get() != openTelemetry) { |
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 removed this check because I think it will always be true because GlobalOpenTelemetry
wraps the instance used in set
.
https://ge.opentelemetry.io/s/ywwfjwk44fa66/tests/task/:instrumentation:spring:spring-boot-autoconfigure:test/details/io.opentelemetry.instrumentation.spring.autoconfigure.internal.instrumentation.scheduling.SchedulingInstrumentationAspectTest/schedules()?top-execution=1