Skip to content

Commit

Permalink
Merge branch 'develop' into ps/test-cleanups
Browse files Browse the repository at this point in the history
# Conflicts:
#	build.gradle
  • Loading branch information
pshapiro4broad committed Sep 9, 2024
2 parents 40461a3 + 6f12b29 commit d178645
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 42 deletions.
20 changes: 10 additions & 10 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ plugins {

id 'com.diffplug.spotless' version '6.25.0'
id 'com.github.ben-manes.versions' version '0.51.0'
id 'com.jfrog.artifactory' version '5.2.3'
id 'com.jfrog.artifactory' version '5.2.5'
id 'org.sonarqube' version '5.1.0.4882'
id 'io.spring.dependency-management' version '1.1.5'
id 'org.springframework.boot' version '3.3.1'
id 'io.spring.dependency-management' version '1.1.6'
id 'org.springframework.boot' version '3.3.3'
id 'com.srcclr.gradle' version '3.1.12'
}

Expand All @@ -21,7 +21,7 @@ project.ext {

// 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.37.0'
ext['opentelemetry.version'] = '1.41.0'

// If true, search local repository (~/.m2/repository/) first for dependencies.
def useMavenLocal = false
Expand Down Expand Up @@ -66,7 +66,7 @@ dependencies {

// Google dependencies
// use common bom
implementation platform('com.google.cloud:libraries-bom:26.44.0')
implementation platform('com.google.cloud:libraries-bom:26.45.0')
implementation group: 'com.google.cloud', name: 'google-cloud-core'
implementation group: 'com.google.cloud', name: 'google-cloud-pubsub'
api group: 'com.google.guava', name: 'guava'
Expand All @@ -77,7 +77,7 @@ dependencies {

// Terra libraries
implementation group: 'org.broadinstitute.dsde.workbench', name: 'sam-client_2.13', version: '0.1-0c4b377'
var stairwayVersion= '1.1.10-SNAPSHOT'
var stairwayVersion= '1.1.12-SNAPSHOT'
api "bio.terra:stairway-gcp:${stairwayVersion}"
implementation "bio.terra:stairway-azure:${stairwayVersion}"

Expand All @@ -87,9 +87,9 @@ dependencies {
implementation group: 'ch.qos.logback.contrib', name: 'logback-jackson', version: '0.1.5'

// OpenTelemetry BOMs (opentelemetry-bom versioned by Spring dependency manager)
implementation platform('io.opentelemetry:opentelemetry-bom-alpha:1.37.0-alpha')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.3.0')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha:2.3.0-alpha')
implementation platform('io.opentelemetry:opentelemetry-bom-alpha:1.41.0-alpha')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom:2.7.0')
implementation platform('io.opentelemetry.instrumentation:opentelemetry-instrumentation-bom-alpha:2.7.0-alpha')
// OpenTelemetry dependencies versioned by BOMs
api 'io.opentelemetry:opentelemetry-api'
implementation 'io.opentelemetry:opentelemetry-sdk'
Expand All @@ -98,7 +98,7 @@ dependencies {
implementation 'io.opentelemetry.instrumentation:opentelemetry-spring-webmvc-6.0'
implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-api'
implementation 'io.opentelemetry.instrumentation:opentelemetry-instrumentation-annotations'
implementation 'io.opentelemetry.instrumentation:opentelemetry-spring-boot'
implementation 'io.opentelemetry.instrumentation:opentelemetry-spring-boot-autoconfigure'
implementation 'io.opentelemetry:opentelemetry-exporter-prometheus'
implementation 'io.opentelemetry:opentelemetry-api-incubator'
implementation 'io.opentelemetry:opentelemetry-sdk-extension-autoconfigure'
Expand Down
2 changes: 1 addition & 1 deletion settings.gradle
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
rootProject.name = 'terra-common-lib'

gradle.ext.projectGroup = 'bio.terra'
gradle.ext.tclVersion = '1.1.18-SNAPSHOT'
gradle.ext.tclVersion = '1.1.19-SNAPSHOT'
29 changes: 16 additions & 13 deletions src/main/java/bio/terra/common/opentelemetry/HttpMetricsAdvice.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@

import io.opentelemetry.api.incubator.metrics.ExtendedDoubleHistogramBuilder;
import io.opentelemetry.api.metrics.DoubleHistogramBuilder;
import io.opentelemetry.semconv.SemanticAttributes;
import io.opentelemetry.semconv.HttpAttributes;
import io.opentelemetry.semconv.NetworkAttributes;
import io.opentelemetry.semconv.ServerAttributes;
import io.opentelemetry.semconv.UrlAttributes;
import java.util.List;
import java.util.stream.DoubleStream;

Expand Down Expand Up @@ -52,13 +55,13 @@ static void applyClientDurationAdvice(DoubleHistogramBuilder builder) {
((ExtendedDoubleHistogramBuilder) builder)
.setAttributesAdvice(
asList(
SemanticAttributes.HTTP_REQUEST_METHOD,
SemanticAttributes.HTTP_RESPONSE_STATUS_CODE,
HttpAttributes.HTTP_REQUEST_METHOD,
HttpAttributes.HTTP_RESPONSE_STATUS_CODE,
ERROR_TYPE,
SemanticAttributes.NETWORK_PROTOCOL_NAME,
SemanticAttributes.NETWORK_PROTOCOL_VERSION,
SemanticAttributes.SERVER_ADDRESS,
SemanticAttributes.SERVER_PORT));
NetworkAttributes.NETWORK_PROTOCOL_NAME,
NetworkAttributes.NETWORK_PROTOCOL_VERSION,
ServerAttributes.SERVER_ADDRESS,
ServerAttributes.SERVER_PORT));
}

static void applyServerDurationAdvice(DoubleHistogramBuilder builder) {
Expand All @@ -68,13 +71,13 @@ static void applyServerDurationAdvice(DoubleHistogramBuilder builder) {
((ExtendedDoubleHistogramBuilder) builder)
.setAttributesAdvice(
asList(
SemanticAttributes.HTTP_ROUTE,
SemanticAttributes.HTTP_REQUEST_METHOD,
SemanticAttributes.HTTP_RESPONSE_STATUS_CODE,
HttpAttributes.HTTP_ROUTE,
HttpAttributes.HTTP_REQUEST_METHOD,
HttpAttributes.HTTP_RESPONSE_STATUS_CODE,
ERROR_TYPE,
SemanticAttributes.NETWORK_PROTOCOL_NAME,
SemanticAttributes.NETWORK_PROTOCOL_VERSION,
SemanticAttributes.URL_SCHEME));
NetworkAttributes.NETWORK_PROTOCOL_NAME,
NetworkAttributes.NETWORK_PROTOCOL_VERSION,
UrlAttributes.URL_SCHEME));
}

private HttpMetricsAdvice() {}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package bio.terra.common.opentelemetry;

import bio.terra.common.tracing.ExcludingUrlSampler;
import io.opentelemetry.instrumentation.spring.autoconfigure.EnableOpenTelemetry;
import io.opentelemetry.sdk.autoconfigure.spi.AutoConfigurationCustomizerProvider;
import io.opentelemetry.sdk.metrics.InstrumentSelector;
import io.opentelemetry.sdk.metrics.View;
Expand All @@ -18,7 +17,6 @@
import org.springframework.data.util.Pair;

@Configuration
@EnableOpenTelemetry
@EnableConfigurationProperties(value = {TracingProperties.class})
public class OpenTelemetryConfig {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import io.opentelemetry.sdk.trace.data.LinkData;
import io.opentelemetry.sdk.trace.samplers.Sampler;
import io.opentelemetry.sdk.trace.samplers.SamplingResult;
import io.opentelemetry.semconv.SemanticAttributes;
import io.opentelemetry.semconv.UrlAttributes;
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -38,8 +38,8 @@ public SamplingResult shouldSample(
// HTTP_TARGET seems to have the right information but that is deprecated
// check URL_PATH to be forward compatible. JUST CHECK THEM ALL
var urlCandidates = new HashSet<String>();
urlCandidates.add(attributes.get(SemanticAttributes.URL_PATH));
urlCandidates.add(attributes.get(SemanticAttributes.HTTP_TARGET));
urlCandidates.add(attributes.get(UrlAttributes.URL_PATH));
urlCandidates.add(attributes.get(UrlAttributes.URL_QUERY));
urlCandidates.add(name);
// removeAll below does not like nulls so remove any if they exist
urlCandidates.remove(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,20 +28,20 @@
@ActiveProfiles("human-readable-logging")
@Tag("unit")
@ExtendWith(OutputCaptureExtension.class)
public class HumanReadableLoggingTest {
class HumanReadableLoggingTest {

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

@BeforeEach
public void setUp() {
void setUp() {
// Ensure the request ID is always set to a known value.
when(requestIdFilter.generateRequestId()).thenReturn("12345");
}

@Test
public void testRequestLogging(CapturedOutput capturedOutput) {
void testRequestLogging(CapturedOutput capturedOutput) {
ResponseEntity<String> response =
testRestTemplate.getForEntity("/testRequestLogging", String.class);
assertThat(response.getStatusCode().value()).isEqualTo(200);
Expand All @@ -55,6 +55,6 @@ public void testRequestLogging(CapturedOutput capturedOutput) {
// in the main logback.xml
assertThat(allOutput).contains("12345");
// Poor-man's check for lack of JSON
assertThat(allOutput).doesNotContain("{");
assertThat(allOutput).doesNotContain("{\"timestampSeconds\":");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,17 @@
import io.opentelemetry.context.Context;
import io.opentelemetry.sdk.trace.samplers.Sampler;
import io.opentelemetry.sdk.trace.samplers.SamplingDecision;
import io.opentelemetry.semconv.SemanticAttributes;
import io.opentelemetry.semconv.UrlAttributes;
import java.util.List;
import java.util.Set;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Tag;
import org.junit.jupiter.api.Test;

@Tag("unit")
public class ExcludingUrlSamplerTest {
class ExcludingUrlSamplerTest {
@Test
public void testShouldSample() {
void testShouldSample() {
var sampler = new ExcludingUrlSampler(Set.of("/foo", "/bar"), Sampler.alwaysOn());
assertThat(
sampler
Expand All @@ -28,7 +28,7 @@ public void testShouldSample() {
}

@Test
public void testShouldNotSampleBecauseDelegateSaidDrop() {
void testShouldNotSampleBecauseDelegateSaidDrop() {
var sampler = new ExcludingUrlSampler(Set.of("/foo", "/bar"), Sampler.alwaysOff());
assertThat(
sampler
Expand All @@ -39,7 +39,7 @@ public void testShouldNotSampleBecauseDelegateSaidDrop() {
}

@Test
public void testShouldNotSampleByName() {
void testShouldNotSampleByName() {
var sampler = new ExcludingUrlSampler(Set.of("/foo", "/bar"), Sampler.alwaysOn());
assertThat(
sampler
Expand All @@ -50,7 +50,7 @@ public void testShouldNotSampleByName() {
}

@Test
public void testShouldNotSampleByUrlPath() {
void testShouldNotSampleByUrlPath() {
// note that the internal implementation of ExcludingUrlSampler has slightly different behavior
// when the size of the excludedUrls is smaller than the size of the urlCandidates. So this test
// has only one excludedUrl to make sure the scenario works. The different behavior comes from
Expand All @@ -63,14 +63,14 @@ public void testShouldNotSampleByUrlPath() {
"",
"/baz",
SpanKind.INTERNAL,
Attributes.of(SemanticAttributes.URL_PATH, "/bar"),
Attributes.of(UrlAttributes.URL_PATH, "/bar"),
List.of())
.getDecision(),
Matchers.is(SamplingDecision.DROP));
}

@Test
public void testShouldNotSampleByHttpTarget() {
void testShouldNotSampleByUrlQuery() {
var sampler = new ExcludingUrlSampler(Set.of("/foo", "/bar"), Sampler.alwaysOn());
assertThat(
sampler
Expand All @@ -79,7 +79,7 @@ public void testShouldNotSampleByHttpTarget() {
"",
"/baz",
SpanKind.INTERNAL,
Attributes.of(SemanticAttributes.HTTP_TARGET, "/bar"),
Attributes.of(UrlAttributes.URL_QUERY, "/bar"),
List.of())
.getDecision(),
Matchers.is(SamplingDecision.DROP));
Expand Down

0 comments on commit d178645

Please sign in to comment.