-
Notifications
You must be signed in to change notification settings - Fork 1k
[Spring Starter] Spring boot 4 support #15459
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
base: main
Are you sure you want to change the base?
[Spring Starter] Spring boot 4 support #15459
Conversation
0949e97 to
330cd89
Compare
|
Would that resolve the missing metrics going to mimir on spring boot 4 setup using the java agent @jaydeluca ? |
|
@George-C-Odes there is no ETA, im working on this as I can. |
|
my goal is to get this all completed before the next release 🤞. This is just the spring boot starter, there are still other javaagent instrumentations that need to be fixed too |
…entelemetry-java-instrumentation into spring-boot-4-autoconfigure
...configure/internal/instrumentation/jdbc/JdbcInstrumentationSpringBoot4AutoConfiguration.java
Show resolved
Hide resolved
...t-4/src/test/java/io/opentelemetry/spring/smoketest/AbstractKafkaSpringStarterSmokeTest.java
Outdated
Show resolved
Hide resolved
...configure/internal/instrumentation/jdbc/JdbcInstrumentationSpringBoot4AutoConfiguration.java
Outdated
Show resolved
Hide resolved
zeitlinger
left a comment
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.
great to see that the code size is getting smaller now 😄
| @ConditionalOnBean({DataSource.class}) | ||
| @ConditionalOnMissingClass( | ||
| "org.springframework.boot.jdbc.autoconfigure.DataSourceAutoConfiguration") // Spring Boot 4+ | ||
| @ConditionalOnClass(DataSourceAutoConfiguration.class) |
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.
can you add a comment back? e.g. "removed in Spring Boot 4"
...utoconfigure/internal/instrumentation/mongo/MongoClientInstrumentationAutoConfiguration.java
Outdated
Show resolved
Hide resolved
| @Configuration(proxyBeanMethods = false) | ||
| @ConditionalOnMissingClass( | ||
| "org.springframework.boot.autoconfigure.jdbc.DataSourceAutoConfiguration") // Spring Boot 2 & 3 | ||
| @ConditionalOnClass(DataSourceAutoConfiguration.class) |
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.
condition looks identical to boot 3
| DefaultKafkaProducerFactoryCustomizer.class | ||
| }) | ||
| @ConditionalOnMissingBean(name = "otelKafkaProducerFactoryCustomizer") | ||
| @ConditionalOnMissingClass( |
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.
why not needed anymore?
| */ | ||
| @ConditionalOnClass({MongoClientSettings.class, MongoClientSettingsBuilderCustomizer.class}) | ||
| @ConditionalOnEnabledInstrumentation(module = "mongo") | ||
| @ConditionalOnMissingClass( |
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.
why is this not needed anymore?
| @Autowired private RestTemplateBuilder restTemplateBuilder; | ||
| @Autowired private JdbcTemplate jdbcTemplate; | ||
|
|
||
| abstract void makeClientCall(); |
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.
great!
| public JdbcInstrumentationSpringBoot4AutoConfiguration() {} | ||
|
|
||
| @Bean | ||
| // static to avoid "is not eligible for getting processed by all BeanPostProcessors" warning |
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.
Not really for this PR but I suspect that (proxyBeanMethods = false) has the same effect as making the method static. If I remember correctly the warning was triggered by proxying the configuration class methods.
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 think you're right - that would be a nice follow-up PR
| @Bean | ||
| DefaultKafkaProducerFactoryCustomizer otelKafkaProducerFactoryCustomizer( | ||
| OpenTelemetry openTelemetry) { | ||
| KafkaTelemetry kafkaTelemetry = KafkaTelemetry.create(openTelemetry); | ||
| return producerFactory -> producerFactory.addPostProcessor(kafkaTelemetry::wrap); | ||
| } |
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.
actually only this bean is different for spring boot 4. Wondering whether it would make sense to move this bean into separate (maybe nested?) configuration class so that the other beans wouldn't need to be copy-pasted. Or is that too much effort?
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.
| JdbcInstrumentationSpringBoot4AutoConfiguration.class, | ||
| DataSourceAutoConfiguration.class)) |
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.
since only there 2 lines are different perhaps it would make sense to introduce a testing module and share code for these tests? idk maybe other tests required more changes.
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.
yeah - I think that could work
|
|
||
| // Exclude Spring Boot specific tests from compilation when testLatestDeps is true | ||
| // These tests are covered by testSpring4 suite | ||
| if (latestDepTest) { |
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.
Sometimes when we have a separate suite for new version we limit the latest dep in base version to 1.+ and in version 2 suite either set the version to 2.0 or latest.release depending on the latest dep flag. Would that also work here? Or maybe it would help if these tests were moved to a separate suite and the main test directory would only contain tests that work on all versions?
| import org.testcontainers.utility.DockerImageName; | ||
|
|
||
| @DisabledInNativeImage // See GraalVmNativeKafkaSpringStarterSmokeTest for the GraalVM native test | ||
| class KafkaSpringStarterSmokeTest extends AbstractKafkaSpringStarterSmokeTest { |
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.
should be able to extend AbstractJvmKafkaSpringStarterSmokeTest if the auto config class is passed to ctor or can be overridden
(same for mongo)
|
|
||
| // JFR based metrics | ||
| for (String metric : | ||
| List.of( |
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 think we can also extract a superclass here to avoid the duplication

Related to #14906
Notes:
I tried to avoid as much duplication as I could, If anyone has suggestions on areas to revisit, I can try again