CORE-69: Spring Boot 3.4.0 and otel cleanup #211
Conversation
davidangb
changed the title
| @@ -51,6 +52,7 @@ dependencies { | |||
|
|
|||
| // Spring | |||
| implementation group: 'org.springframework.retry', name: 'spring-retry' | |||
| implementation group: 'org.springframework.boot', name: 'spring-boot-autoconfigure' | |||
| implementation group: 'org.springframework.boot', name: 'spring-boot-starter-data-jdbc' | |||
| implementation group: 'org.springframework.boot', name: 'spring-boot-starter-web' | |||
There was a problem hiding this comment.
spring-boot-autoconfigure moved up from line 108, where it was hidden in with otel dependencies
build.gradle
Outdated
| // We need >= 1.32.0 so that our HttpServerMetrics can use Meter.setExplicitBucketBoundariesAdvice: | ||
| ext['opentelemetry.version'] = '1.42.1' | ||
| ext['opentelemetry.version'] = '1.43.0' | ||
| ext['opentelemetry.instrumentation.version'] = '2.9.0' // 2.9.0 targets opentelemetry 1.43.0 |
There was a problem hiding this comment.
this is actually a downgrade. We had been on 2.10.0 before this PR. I thought this was more appropriate, since 2.10.0 wants opentelemetry 1.44.1. See https://github.com/open-telemetry/opentelemetry-java-instrumentation/releases.
If we decide to keep all the otel dependency versions aligned with Spring Boot, we should probably tell dependabot to ignore those updates.
There was a problem hiding this comment.
do we need these lines anymore? I think they were there to specify a more advanced version than spring required, is that still true?
| implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure' | ||
| implementation 'io.opentelemetry:opentelemetry-exporter-otlp' | ||
| implementation 'org.springframework.boot:spring-boot-autoconfigure' | ||
| implementation 'io.opentelemetry:opentelemetry-exporter-prometheus' |
There was a problem hiding this comment.
I reorganized this whole section. The diff is hard to read, so I'll try to summarize:
-
Moved the version number for
opentelemetry-instrumentation-bom-alphainto anextvariable at line 26, so it is easily visible next to the main opentelemetry version. -
removed a few dependencies which were redundant; these are pulled in transitively when needed:
implementation platform('io.opentelemetry:opentelemetry-bom-alpha:1.44.1-alpha')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.10.0')
implementation 'io.opentelemetry:opentelemetry-sdk'
implementation 'io.opentelemetry:opentelemetry-sdk-metrics'
-
replaced
'io.opentelemetry.instrumentation:opentelemetry-spring-webmvc-6.0'with'io.opentelemetry.semconv:opentelemetry-semconv'. The latter is smaller; we didn't really need the former. -
grouped into two categories: those version-managed by Spring and those version-managed by us via
opentelemetry-instrumentation-bom-alpha -
within those groupings, alphabetized the libraries
| @@ -77,7 +79,7 @@ dependencies { | |||
| runtimeOnly group: 'org.postgresql', name: 'postgresql' | |||
|
|
|||
| // Terra libraries | |||
| implementation group: 'org.broadinstitute.dsde.workbench', name: 'sam-client_2.13', version: '0.1-0c4b377' | |||
| implementation group: 'org.broadinstitute.dsde.workbench', name: 'sam-client_2.13', version: 'v0.0.329' | |||
There was a problem hiding this comment.
this isn't required by the Spring Boot upgrade, but while in this file I noticed that we were using an old, non-semantic-version of sam-client.
sam-client:0.1-0c4b377 - Feb 14 2024
sam-client:v0.0.329 - Dec 10 2024
davidangb
requested review from
dvoet,
a team and
calypsomatic
and removed request for
a team
|
@dvoet I'd especially love your opinion on the otel library-version changes here |
build.gradle
Outdated
| // We need >= 1.32.0 so that our HttpServerMetrics can use Meter.setExplicitBucketBoundariesAdvice: | ||
| ext['opentelemetry.version'] = '1.42.1' | ||
| ext['opentelemetry.version'] = '1.43.0' | ||
| ext['opentelemetry.instrumentation.version'] = '2.9.0' // 2.9.0 targets opentelemetry 1.43.0 |
There was a problem hiding this comment.
do we need these lines anymore? I think they were there to specify a more advanced version than spring required, is that still true?
@dvoet I suspect we don't need them, but I was worried I missed some code somewhere that referenced that property. I will (try to) remove them. |
|
@SpyBean, which is now deprecatedsam-clientto latestThis PR supersedes #210
References:
https://docs.spring.io/spring-boot/appendix/dependency-versions/coordinates.html
https://github.com/spring-projects/spring-boot/wiki/Spring-Boot-3.4-Release-Notes#deprecation-of-mockbean-and-spybean