From 638dda9f2d60dc3fb337316e3a7730bd1dcaf2f5 Mon Sep 17 00:00:00 2001 From: Sky Rubenstein Date: Thu, 16 Jan 2025 15:46:57 -0500 Subject: [PATCH 1/4] Move to request scoped metrics --- .../terra/app/usermetrics/UserLoggingMetrics.java | 15 ++++++++------- .../usermetrics/UserMetricsInterceptorTest.java | 1 - 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java index dad561c450..427f2e2a83 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java +++ b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java @@ -1,6 +1,9 @@ package bio.terra.app.usermetrics; +import static org.springframework.web.context.WebApplicationContext.SCOPE_REQUEST; + import java.util.HashMap; +import org.springframework.context.annotation.Scope; import org.springframework.stereotype.Component; /** @@ -11,10 +14,10 @@ * API request will get grouped together even if they are set in different methods. */ @Component +@Scope(SCOPE_REQUEST) public class UserLoggingMetrics { - private static final ThreadLocal> metrics = - ThreadLocal.withInitial(() -> new HashMap<>()); + private final HashMap metrics = new HashMap<>(); /** * Get the current thread's metrics instance. If no metrics have been set, return the default @@ -23,7 +26,7 @@ public class UserLoggingMetrics { * @return HashMap metrics */ public HashMap get() { - return metrics.get(); + return metrics; } /** @@ -31,7 +34,7 @@ public HashMap get() { * this key it will be replaced. */ public void set(String key, Object value) { - metrics.get().put(key, value); + metrics.put(key, value); } /** @@ -41,8 +44,6 @@ public void set(String key, Object value) { * @param value HashMap of metrics to add */ public void setAll(HashMap value) { - HashMap properties = metrics.get(); - properties.putAll(value); - metrics.set(properties); + metrics.putAll(value); } } diff --git a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java index be2bba6825..2f546c8a87 100644 --- a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java +++ b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java @@ -70,7 +70,6 @@ class UserMetricsInterceptorTest { @BeforeEach void setUp() { - eventProperties.get().clear(); when(metricsConfig.ignorePaths()).thenReturn(List.of()); when(metricsConfig.appId()).thenReturn(APP_ID); when(metricsConfig.bardBasePath()).thenReturn(BARD_BASE_PATH); From 07053b20a468291508682a333496556d299b0e6f Mon Sep 17 00:00:00 2001 From: Sky Rubenstein Date: Thu, 16 Jan 2025 16:10:25 -0500 Subject: [PATCH 2/4] Add proxymode and remove eventProperties to inline --- .../java/bio/terra/app/usermetrics/UserLoggingMetrics.java | 3 ++- .../bio/terra/app/usermetrics/UserMetricsInterceptor.java | 7 +------ .../terra/app/usermetrics/UserMetricsInterceptorTest.java | 1 - 3 files changed, 3 insertions(+), 8 deletions(-) diff --git a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java index 427f2e2a83..c5ca23cc8f 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java +++ b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java @@ -4,6 +4,7 @@ import java.util.HashMap; import org.springframework.context.annotation.Scope; +import org.springframework.context.annotation.ScopedProxyMode; import org.springframework.stereotype.Component; /** @@ -14,7 +15,7 @@ * API request will get grouped together even if they are set in different methods. */ @Component -@Scope(SCOPE_REQUEST) +@Scope(value = SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS) public class UserLoggingMetrics { private final HashMap metrics = new HashMap<>(); diff --git a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java index 1771203094..ad38bcac88 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java +++ b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java @@ -26,7 +26,6 @@ public class UserMetricsInterceptor implements HandlerInterceptor { private final ApplicationConfiguration applicationConfiguration; private final UserMetricsConfiguration metricsConfig; private final ExecutorService metricsPerformanceThreadpool; - private final UserLoggingMetrics eventProperties; @Autowired public UserMetricsInterceptor( @@ -34,14 +33,12 @@ public UserMetricsInterceptor( AuthenticatedUserRequestFactory authenticatedUserRequestFactory, ApplicationConfiguration applicationConfiguration, UserMetricsConfiguration metricsConfig, - UserLoggingMetrics eventProperties, @Qualifier("metricsReportingThreadpool") ExecutorService metricsPerformanceThreadpool) { this.bardClient = bardClient; this.authenticatedUserRequestFactory = authenticatedUserRequestFactory; this.applicationConfiguration = applicationConfiguration; this.metricsConfig = metricsConfig; this.metricsPerformanceThreadpool = metricsPerformanceThreadpool; - this.eventProperties = eventProperties; } @Override @@ -69,8 +66,6 @@ public void afterCompletion( BardEventProperties.PATH_FIELD_NAME, path)); addToPropertiesIfPresentInHeader( request, properties, "X-Transaction-Id", BardEventProperties.TRANSACTION_ID_FIELD_NAME); - eventProperties.setAll(properties); - HashMap bardEventProperties = eventProperties.get(); // Spawn a thread so that sending the metric doesn't slow down the initial request metricsPerformanceThreadpool.submit( @@ -79,7 +74,7 @@ public void afterCompletion( userRequest, new BardEvent( API_EVENT_NAME, - bardEventProperties, + properties, metricsConfig.appId(), applicationConfiguration.getDnsName()))); } diff --git a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java index 2f546c8a87..d96c75ed08 100644 --- a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java +++ b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java @@ -87,7 +87,6 @@ void setUp() { authenticatedUserRequestFactory, applicationConfiguration, metricsConfig, - eventProperties, metricsPerformanceThreadpool); } From ec4341ccc8b5391d12758d4f1d9cf94a4b1edf9d Mon Sep 17 00:00:00 2001 From: Sky Rubenstein Date: Thu, 16 Jan 2025 16:28:01 -0500 Subject: [PATCH 3/4] Fix test? --- .../bio/terra/app/usermetrics/UserMetricsInterceptor.java | 4 ++++ .../bio/terra/app/usermetrics/UserMetricsInterceptorTest.java | 1 + 2 files changed, 5 insertions(+) diff --git a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java index ad38bcac88..8c917dba13 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java +++ b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java @@ -25,6 +25,7 @@ public class UserMetricsInterceptor implements HandlerInterceptor { private final AuthenticatedUserRequestFactory authenticatedUserRequestFactory; private final ApplicationConfiguration applicationConfiguration; private final UserMetricsConfiguration metricsConfig; + private final UserLoggingMetrics userLoggingMetrics; private final ExecutorService metricsPerformanceThreadpool; @Autowired @@ -33,11 +34,13 @@ public UserMetricsInterceptor( AuthenticatedUserRequestFactory authenticatedUserRequestFactory, ApplicationConfiguration applicationConfiguration, UserMetricsConfiguration metricsConfig, + UserLoggingMetrics userLoggingMetrics, @Qualifier("metricsReportingThreadpool") ExecutorService metricsPerformanceThreadpool) { this.bardClient = bardClient; this.authenticatedUserRequestFactory = authenticatedUserRequestFactory; this.applicationConfiguration = applicationConfiguration; this.metricsConfig = metricsConfig; + this.userLoggingMetrics = userLoggingMetrics; this.metricsPerformanceThreadpool = metricsPerformanceThreadpool; } @@ -66,6 +69,7 @@ public void afterCompletion( BardEventProperties.PATH_FIELD_NAME, path)); addToPropertiesIfPresentInHeader( request, properties, "X-Transaction-Id", BardEventProperties.TRANSACTION_ID_FIELD_NAME); + properties.putAll(userLoggingMetrics.get()); // Spawn a thread so that sending the metric doesn't slow down the initial request metricsPerformanceThreadpool.submit( diff --git a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java index d96c75ed08..2f546c8a87 100644 --- a/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java +++ b/src/test/java/bio/terra/app/usermetrics/UserMetricsInterceptorTest.java @@ -87,6 +87,7 @@ void setUp() { authenticatedUserRequestFactory, applicationConfiguration, metricsConfig, + eventProperties, metricsPerformanceThreadpool); } From 32d774269335758a98719d2393b39ddf1d06a5c0 Mon Sep 17 00:00:00 2001 From: Sky Rubenstein Date: Fri, 17 Jan 2025 11:18:27 -0500 Subject: [PATCH 4/4] PR feedback --- .../app/usermetrics/UserLoggingMetrics.java | 22 +++++++++---------- .../usermetrics/UserMetricsInterceptor.java | 12 +++++----- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java index c5ca23cc8f..c13b1d0a29 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java +++ b/src/main/java/bio/terra/app/usermetrics/UserLoggingMetrics.java @@ -1,11 +1,9 @@ package bio.terra.app.usermetrics; -import static org.springframework.web.context.WebApplicationContext.SCOPE_REQUEST; - import java.util.HashMap; -import org.springframework.context.annotation.Scope; -import org.springframework.context.annotation.ScopedProxyMode; +import java.util.Map; import org.springframework.stereotype.Component; +import org.springframework.web.context.annotation.RequestScope; /** * This class wraps a ThreadLocal variable to store API request properties to log (e.g. method, @@ -15,24 +13,24 @@ * API request will get grouped together even if they are set in different methods. */ @Component -@Scope(value = SCOPE_REQUEST, proxyMode = ScopedProxyMode.TARGET_CLASS) +@RequestScope public class UserLoggingMetrics { - private final HashMap metrics = new HashMap<>(); + private final Map metrics = new HashMap<>(); /** * Get the current thread's metrics instance. If no metrics have been set, return the default * empty HashMap. * - * @return HashMap metrics + * @return Map metrics */ - public HashMap get() { + public Map get() { return metrics; } /** - * Add a new value to the current thread's metrics map. If the map already contains a value for - * this key it will be replaced. + * Add a new value to the metrics map. If the map already contains a value for this key it will be + * replaced. */ public void set(String key, Object value) { metrics.put(key, value); @@ -42,9 +40,9 @@ public void set(String key, Object value) { * Add multiple values to the current thread's metrics map. Any existing values with the same key * will get replaced. * - * @param value HashMap of metrics to add + * @param value Map of metrics to add */ - public void setAll(HashMap value) { + public void setAll(Map value) { metrics.putAll(value); } } diff --git a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java index 8c917dba13..8b84bf706e 100644 --- a/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java +++ b/src/main/java/bio/terra/app/usermetrics/UserMetricsInterceptor.java @@ -61,15 +61,13 @@ public void afterCompletion( if (StringUtils.isEmpty(metricsConfig.bardBasePath()) || ignoreEventForPath(path)) { return; } - - HashMap properties = - new HashMap<>( - Map.of( - BardEventProperties.METHOD_FIELD_NAME, method, - BardEventProperties.PATH_FIELD_NAME, path)); + Map properties = new HashMap<>(userLoggingMetrics.get()); + properties.putAll( + Map.of( + BardEventProperties.METHOD_FIELD_NAME, method, + BardEventProperties.PATH_FIELD_NAME, path)); addToPropertiesIfPresentInHeader( request, properties, "X-Transaction-Id", BardEventProperties.TRANSACTION_ID_FIELD_NAME); - properties.putAll(userLoggingMetrics.get()); // Spawn a thread so that sending the metric doesn't slow down the initial request metricsPerformanceThreadpool.submit(