From d5811699d22465d66e57f137037dcb49570d7b87 Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Thu, 26 Dec 2024 07:46:32 +0000 Subject: [PATCH 1/7] gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case. --- .../io/grpc/gcp/observability/GcpObservabilityTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index 40f2fb01490..3e44b1d0c83 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -196,9 +196,12 @@ public void run() { mock(InternalLoggingServerInterceptor.Factory.class); when(serverInterceptorFactory.create()).thenReturn(serverInterceptor); - try (GcpObservability unused = - GcpObservability.grpcInit( - sink, config, channelInterceptorFactory, serverInterceptorFactory)) { + try { + GcpObservability gcpObservability = GcpObservability.grpcInit( + sink, config, channelInterceptorFactory, serverInterceptorFactory); + // Added the assert statement to fix the PR build/check warnings for + // unused variable gcpObservability. + assertThat(gcpObservability).isNotNull(); List configurators = InternalConfiguratorRegistry.getConfigurators(); assertThat(configurators).hasSize(1); ObservabilityConfigurator configurator = (ObservabilityConfigurator) configurators.get(0); From 8311f006e7f38dc6ecf410a51949754771ae5c3b Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Thu, 2 Jan 2025 05:53:37 +0000 Subject: [PATCH 2/7] gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case. --- .../gcp/observability/GcpObservability.java | 15 +++++++++++ .../observability/GcpObservabilityTest.java | 26 ++++++++++++++++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index 497a1eda30f..cab3c3f1e18 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -146,6 +146,21 @@ public void close() { } } + public void closeWithSleepTime(long sleepTime) { + synchronized (GcpObservability.class) { + sink.close(); + if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) { + try { + Thread.sleep(sleepTime); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + logger.log(Level.SEVERE, "Caught exception during sleep", e); + } + } + instance = null; + } + } + // TODO(dnvindhya): Remove InterceptorFactory and replace with respective // interceptors private void setProducer( diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index 3e44b1d0c83..f4dc0db2867 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -199,9 +199,6 @@ public void run() { try { GcpObservability gcpObservability = GcpObservability.grpcInit( sink, config, channelInterceptorFactory, serverInterceptorFactory); - // Added the assert statement to fix the PR build/check warnings for - // unused variable gcpObservability. - assertThat(gcpObservability).isNotNull(); List configurators = InternalConfiguratorRegistry.getConfigurators(); assertThat(configurators).hasSize(1); ObservabilityConfigurator configurator = (ObservabilityConfigurator) configurators.get(0); @@ -211,6 +208,7 @@ public void run() { assertThat(list.get(2)).isInstanceOf(ConditionalClientInterceptor.class); assertThat(configurator.serverInterceptors).hasSize(1); assertThat(configurator.tracerFactories).hasSize(2); + gcpObservability.closeWithSleepTime(3000); } catch (Exception e) { fail("Encountered exception: " + e); } @@ -264,4 +262,26 @@ public ServerCall.Listener interceptCall( return next.startCall(call, headers); } } + + @Test + public void closeWithSleepTime() throws Exception { + long sleepTime = 1000L; + Sink sink = mock(Sink.class); + ObservabilityConfig config = mock(ObservabilityConfig.class); + when(config.isEnableCloudLogging()).thenReturn(false); + when(config.isEnableCloudMonitoring()).thenReturn(false); + when(config.isEnableCloudTracing()).thenReturn(false); + when(config.getSampler()).thenReturn(Samplers.neverSample()); + + InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = + mock(InternalLoggingChannelInterceptor.Factory.class); + InternalLoggingServerInterceptor.Factory serverInterceptorFactory = + mock(InternalLoggingServerInterceptor.Factory.class); + GcpObservability gcpObservability = + GcpObservability.grpcInit( + sink, config, channelInterceptorFactory, serverInterceptorFactory); + gcpObservability.closeWithSleepTime(sleepTime); + verify(sink).close(); + } + } From d8e674142804454e753ddb9de9ec25d4cb0083f4 Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Thu, 2 Jan 2025 07:10:27 +0000 Subject: [PATCH 3/7] gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case - fixed review comments. --- .../gcp/observability/GcpObservabilityTest.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index f4dc0db2867..a22d6949e08 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -263,15 +263,16 @@ public ServerCall.Listener interceptCall( } } + // Test to verify the closeWithSleepTime method along with sleep time explicitly. @Test public void closeWithSleepTime() throws Exception { long sleepTime = 1000L; Sink sink = mock(Sink.class); - ObservabilityConfig config = mock(ObservabilityConfig.class); - when(config.isEnableCloudLogging()).thenReturn(false); - when(config.isEnableCloudMonitoring()).thenReturn(false); - when(config.isEnableCloudTracing()).thenReturn(false); - when(config.getSampler()).thenReturn(Samplers.neverSample()); + ObservabilityConfig observabilityConfig = mock(ObservabilityConfig.class); + when(observabilityConfig.isEnableCloudLogging()).thenReturn(false); + when(observabilityConfig.isEnableCloudMonitoring()).thenReturn(false); + when(observabilityConfig.isEnableCloudTracing()).thenReturn(false); + when(observabilityConfig.getSampler()).thenReturn(Samplers.neverSample()); InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = mock(InternalLoggingChannelInterceptor.Factory.class); @@ -279,9 +280,8 @@ public void closeWithSleepTime() throws Exception { mock(InternalLoggingServerInterceptor.Factory.class); GcpObservability gcpObservability = GcpObservability.grpcInit( - sink, config, channelInterceptorFactory, serverInterceptorFactory); + sink, observabilityConfig, channelInterceptorFactory, serverInterceptorFactory); gcpObservability.closeWithSleepTime(sleepTime); verify(sink).close(); } - } From f0d4344f9465b65c7266c44b6366f3e100738ceb Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Thu, 2 Jan 2025 08:59:22 +0000 Subject: [PATCH 4/7] gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case - fixed review comments. --- .../java/io/grpc/gcp/observability/GcpObservability.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index cab3c3f1e18..450770faeb1 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -146,6 +146,11 @@ public void close() { } } + /** + * Method to close along with sleep time explicitly. + * + * @param sleepTime sleepTime + */ public void closeWithSleepTime(long sleepTime) { synchronized (GcpObservability.class) { sink.close(); From e7289e5e8802b74de84c67e2a260ab1622ee4a0b Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Wed, 8 Jan 2025 09:58:53 +0000 Subject: [PATCH 5/7] gcp-observability: Fixed review comments for GcpObservabilityTest.enableObservability test case. --- .../io/grpc/gcp/observability/GcpObservability.java | 8 ++++++-- .../grpc/gcp/observability/GcpObservabilityTest.java | 12 +++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index 450770faeb1..e2cad28fabe 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -151,12 +151,16 @@ public void close() { * * @param sleepTime sleepTime */ - public void closeWithSleepTime(long sleepTime) { + protected void closeWithSleepTime(long sleepTime, TimeUnit timeUnit) { synchronized (GcpObservability.class) { + if (instance == null) { + throw new IllegalStateException("GcpObservability already closed!"); + } sink.close(); if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) { try { - Thread.sleep(sleepTime); + long sleepTimeMillis = TimeUnit.MILLISECONDS.convert(sleepTime, timeUnit); + Thread.sleep(sleepTimeMillis); } catch (InterruptedException e) { Thread.currentThread().interrupt(); logger.log(Level.SEVERE, "Caught exception during sleep", e); diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index a22d6949e08..c5395004a03 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -49,6 +49,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.util.concurrent.TimeUnit; @RunWith(JUnit4.class) public class GcpObservabilityTest { @@ -208,7 +209,13 @@ public void run() { assertThat(list.get(2)).isInstanceOf(ConditionalClientInterceptor.class); assertThat(configurator.serverInterceptors).hasSize(1); assertThat(configurator.tracerFactories).hasSize(2); - gcpObservability.closeWithSleepTime(3000); + gcpObservability.closeWithSleepTime(3000, TimeUnit.MILLISECONDS); + try { + gcpObservability.close(); + fail("Expected IllegalStateException to be thrown on second close"); + } catch (IllegalStateException e) { + assertThat(e.getMessage()).isEqualTo("GcpObservability already closed!"); + } } catch (Exception e) { fail("Encountered exception: " + e); } @@ -281,7 +288,6 @@ public void closeWithSleepTime() throws Exception { GcpObservability gcpObservability = GcpObservability.grpcInit( sink, observabilityConfig, channelInterceptorFactory, serverInterceptorFactory); - gcpObservability.closeWithSleepTime(sleepTime); - verify(sink).close(); + gcpObservability.closeWithSleepTime(3000, TimeUnit.MILLISECONDS); } } From 3d2e3ba406fedbcfaac1cd1e611b4ce17742fcbf Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Tue, 21 Jan 2025 14:18:22 +0000 Subject: [PATCH 6/7] gcp-observability: Fixed some review points and others in-Progress. --- .../gcp/observability/GcpObservability.java | 2 +- .../observability/GcpObservabilityTest.java | 34 ++++--------------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index e2cad28fabe..1ff0e98b328 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -151,7 +151,7 @@ public void close() { * * @param sleepTime sleepTime */ - protected void closeWithSleepTime(long sleepTime, TimeUnit timeUnit) { + void closeWithSleepTime(long sleepTime, TimeUnit timeUnit) { synchronized (GcpObservability.class) { if (instance == null) { throw new IllegalStateException("GcpObservability already closed!"); diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index c5395004a03..9ab1c03bedf 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -210,15 +210,16 @@ public void run() { assertThat(configurator.serverInterceptors).hasSize(1); assertThat(configurator.tracerFactories).hasSize(2); gcpObservability.closeWithSleepTime(3000, TimeUnit.MILLISECONDS); - try { - gcpObservability.close(); - fail("Expected IllegalStateException to be thrown on second close"); - } catch (IllegalStateException e) { - assertThat(e.getMessage()).isEqualTo("GcpObservability already closed!"); - } + // try { + // gcpObservability.close(); + // fail("Expected IllegalStateException to be thrown on second close"); + // } catch (IllegalStateException e) { + // assertThat(e.getMessage()).isEqualTo("GcpObservability already closed!"); + // } } catch (Exception e) { fail("Encountered exception: " + e); } + verify(sink).close(); } } @@ -269,25 +270,4 @@ public ServerCall.Listener interceptCall( return next.startCall(call, headers); } } - - // Test to verify the closeWithSleepTime method along with sleep time explicitly. - @Test - public void closeWithSleepTime() throws Exception { - long sleepTime = 1000L; - Sink sink = mock(Sink.class); - ObservabilityConfig observabilityConfig = mock(ObservabilityConfig.class); - when(observabilityConfig.isEnableCloudLogging()).thenReturn(false); - when(observabilityConfig.isEnableCloudMonitoring()).thenReturn(false); - when(observabilityConfig.isEnableCloudTracing()).thenReturn(false); - when(observabilityConfig.getSampler()).thenReturn(Samplers.neverSample()); - - InternalLoggingChannelInterceptor.Factory channelInterceptorFactory = - mock(InternalLoggingChannelInterceptor.Factory.class); - InternalLoggingServerInterceptor.Factory serverInterceptorFactory = - mock(InternalLoggingServerInterceptor.Factory.class); - GcpObservability gcpObservability = - GcpObservability.grpcInit( - sink, observabilityConfig, channelInterceptorFactory, serverInterceptorFactory); - gcpObservability.closeWithSleepTime(3000, TimeUnit.MILLISECONDS); - } } From f481f927f6ed988148443756339f0b3951c25942 Mon Sep 17 00:00:00 2001 From: harshagoo94 Date: Wed, 22 Jan 2025 13:03:49 +0000 Subject: [PATCH 7/7] gcp-obsrevability: Fixed all review comments. --- .../gcp/observability/GcpObservability.java | 21 ++----------------- .../observability/GcpObservabilityTest.java | 6 ------ 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java index 1ff0e98b328..3ee14c19ef2 100644 --- a/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java +++ b/gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java @@ -127,23 +127,7 @@ static GcpObservability grpcInit( /** Un-initialize/shutdown grpc-observability. */ @Override public void close() { - synchronized (GcpObservability.class) { - if (instance == null) { - throw new IllegalStateException("GcpObservability already closed!"); - } - sink.close(); - if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) { - try { - // Sleeping before shutdown to ensure all metrics and traces are flushed - Thread.sleep( - TimeUnit.MILLISECONDS.convert(2 * METRICS_EXPORT_INTERVAL, TimeUnit.SECONDS)); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - logger.log(Level.SEVERE, "Caught exception during sleep", e); - } - } - instance = null; - } + closeWithSleepTime(2 * METRICS_EXPORT_INTERVAL, TimeUnit.SECONDS); } /** @@ -159,8 +143,7 @@ void closeWithSleepTime(long sleepTime, TimeUnit timeUnit) { sink.close(); if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) { try { - long sleepTimeMillis = TimeUnit.MILLISECONDS.convert(sleepTime, timeUnit); - Thread.sleep(sleepTimeMillis); + timeUnit.sleep(sleepTime); } catch (InterruptedException e) { Thread.currentThread().interrupt(); logger.log(Level.SEVERE, "Caught exception during sleep", e); diff --git a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java index 9ab1c03bedf..3c9ecb8d8fe 100644 --- a/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java +++ b/gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java @@ -210,12 +210,6 @@ public void run() { assertThat(configurator.serverInterceptors).hasSize(1); assertThat(configurator.tracerFactories).hasSize(2); gcpObservability.closeWithSleepTime(3000, TimeUnit.MILLISECONDS); - // try { - // gcpObservability.close(); - // fail("Expected IllegalStateException to be thrown on second close"); - // } catch (IllegalStateException e) { - // assertThat(e.getMessage()).isEqualTo("GcpObservability already closed!"); - // } } catch (Exception e) { fail("Encountered exception: " + e); }