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.

Sign up for GitHub

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

CORE-69: Spring Boot 3.4.0 and otel cleanup #211

Merged
merged 6 commits into from
Dec 11, 2024
Merged

CORE-69: Spring Boot 3.4.0 and otel cleanup #211

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5cad5d0
align otel dependencies
davidangb Dec 11, 2024
8bc578c
remove deprecated @SpyBean
davidangb Dec 11, 2024
e12fb79
update code comment
davidangb Dec 11, 2024
f91a921
spotless
davidangb Dec 11, 2024
f2de8c6
update sam client
davidangb Dec 11, 2024
a0125f0
inline otel versions
davidangb Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 16 additions & 19 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ plugins {
id 'com.jfrog.artifactory' version '5.2.5'
id 'org.sonarqube' version '5.1.0.4882'
id 'io.spring.dependency-management' version '1.1.6'
id 'org.springframework.boot' version '3.3.4'
id 'org.springframework.boot' version '3.4.0'
id 'ru.vyarus.quality' version '5.0.0'
id 'com.srcclr.gradle' version '3.1.12'
}
Expand All @@ -20,9 +20,10 @@ project.ext {
isCiServer = System.getenv().containsKey("CI")
}

// Spring Boot 3.2.3 pulls in opentelemetry-bom 1.31.0.
// We need >= 1.32.0 so that our HttpServerMetrics can use Meter.setExplicitBucketBoundariesAdvice:
ext['opentelemetry.version'] = '1.42.1'
// Spring Boot 3.4.0 pulls in opentelemetry-bom 1.43.0.
// When upgrading Spring Boot, re-check these versions.
ext['opentelemetry.version'] = '1.43.0'
ext['opentelemetry.instrumentation.version'] = '2.9.0' // 2.9.0 targets opentelemetry 1.43.0
Copy link
Contributor Author

@davidangb davidangb Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need these lines anymore? I think they were there to specify a more advanced version than spring required, is that still true?


// If true, search local repository (~/.m2/repository/) first for dependencies.
def useMavenLocal = false
Expand Down Expand Up @@ -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'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

spring-boot-autoconfigure moved up from line 108, where it was hidden in with otel dependencies

annotationProcessor group: 'org.springframework.boot', name: 'spring-boot-configuration-processor'
Expand All @@ -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'
Copy link
Contributor Author

@davidangb davidangb Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

var stairwayVersion= '1.1.15-SNAPSHOT'
api "bio.terra:stairway-gcp:${stairwayVersion}"
implementation "bio.terra:stairway-azure:${stairwayVersion}"
Expand All @@ -87,25 +89,20 @@ dependencies {
implementation group: 'ch.qos.logback.contrib', name: 'logback-json-classic', version: '0.1.5'
implementation group: 'ch.qos.logback.contrib', name: 'logback-jackson', version: '0.1.5'

// OpenTelemetry BOMs (opentelemetry-bom versioned by Spring dependency manager)
// If the following versions get updated, be sure to update line 25 for ext['opentelemetry.version']
implementation platform('io.opentelemetry:opentelemetry-bom-alpha:1.44.1-alpha')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.10.0')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha:2.10.0-alpha')
// OpenTelemetry dependencies versioned by BOMs
// OpenTelemetry dependencies:
implementation platform("io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha:${project.ext['opentelemetry.instrumentation.version']}-alpha")
// ... versioned by Spring Boot
api 'io.opentelemetry:opentelemetry-api'
implementation 'io.opentelemetry:opentelemetry-sdk'
implementation 'io.opentelemetry:opentelemetry-sdk-metrics'
implementation 'io.opentelemetry:opentelemetry-exporter-logging'
implementation 'io.opentelemetry.instrumentation:opentelemetry-spring-webmvc-6.0'
implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-api'
implementation 'io.opentelemetry:opentelemetry-exporter-otlp'
implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
// ... versioned by opentelemetry-instrumentation-bom-alpha
implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations'
implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-api'
implementation 'io.opentelemetry.instrumentation:opentelemetry-spring-boot-autoconfigure'
implementation 'io.opentelemetry:opentelemetry-exporter-prometheus'
implementation 'io.opentelemetry.semconv:opentelemetry-semconv'
implementation 'io.opentelemetry:opentelemetry-api-incubator'
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'
Copy link
Contributor Author

@davidangb davidangb Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I reorganized this whole section. The diff is hard to read, so I'll try to summarize:

  1. Moved the version number for opentelemetry-instrumentation-bom-alpha into an ext variable at line 26, so it is easily visible next to the main opentelemetry version.

  2. 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'

  1. 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.

  2. grouped into two categories: those version-managed by Spring and those version-managed by us via opentelemetry-instrumentation-bom-alpha

  3. within those groupings, alphabetized the libraries


// Google cloud open telemetry exporters
implementation 'com.google.cloud.opentelemetry:exporter-trace:0.33.0'
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/bio/terra/common/logging/HumanReadableLoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.boot.test.system.CapturedOutput;
import org.springframework.boot.test.system.OutputCaptureExtension;
import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.http.ResponseEntity;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.bean.override.mockito.MockitoSpyBean;

/**
* A test to verify that the human-readable-logging Spring Profile will disable the configuration of
Expand All @@ -32,7 +32,7 @@ class HumanReadableLoggingTest {

@Autowired private TestRestTemplate testRestTemplate;
// Spy bean to allow us to mock out the RequestIdFilter ID generator.
@SpyBean private RequestIdFilter requestIdFilter;
@MockitoSpyBean private RequestIdFilter requestIdFilter;

@BeforeEach
void setUp() {
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/bio/terra/common/logging/LoggingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,12 @@
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.context.SpringBootTest;
import org.springframework.boot.test.context.SpringBootTest.WebEnvironment;
import org.springframework.boot.test.mock.mockito.SpyBean;
import org.springframework.boot.test.system.CapturedOutput;
import org.springframework.boot.test.system.OutputCaptureExtension;
import org.springframework.boot.test.web.client.TestRestTemplate;
import org.springframework.http.ResponseEntity;
import org.springframework.test.context.ActiveProfiles;
import org.springframework.test.context.bean.override.mockito.MockitoSpyBean;
import org.springframework.test.context.junit.jupiter.SpringJUnitConfig;

/**
Expand Down Expand Up @@ -55,7 +55,7 @@ public class LoggingTest {

@Autowired private TestRestTemplate testRestTemplate;
// Spy bean to allow us to mock out the RequestIdFilter ID generator.
@SpyBean private RequestIdFilter requestIdFilter;
@MockitoSpyBean private RequestIdFilter requestIdFilter;

@BeforeEach
public void setUp() throws IOException, ServletException {
Expand Down
Loading