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.

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

gcp-observability: Fixed slowness for GcpObservabilityTest.enableObservability test case #11783

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,30 @@ public void close() {
}
}

/**
* Method to close along with sleep time explicitly.
*
* @param sleepTime sleepTime
*/
protected void closeWithSleepTime(long sleepTime, TimeUnit timeUnit) {
synchronized (GcpObservability.class) {
if (instance == null) {
throw new IllegalStateException("GcpObservability already closed!");
}
sink.close();
Copy link
Member

Choose a reason for hiding this comment

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

Add the missing instance == null check, and then have the old method call this new method with a hard-coded delay.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

Copy link
Member

Choose a reason for hiding this comment

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

You did half of what I said, but the old method isn't calling the new method.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved and passing the delay of 1 minute (existing value).

if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) {
try {
long sleepTimeMillis = TimeUnit.MILLISECONDS.convert(sleepTime, timeUnit);
Copy link
Member

Choose a reason for hiding this comment

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

timeUnit.sleep(sleepTime)

Copy link
Author

Choose a reason for hiding this comment

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

Resolved

Thread.sleep(sleepTimeMillis);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
logger.log(Level.SEVERE, "Caught exception during sleep", e);
}
}
instance = null;
}
}

// TODO(dnvindhya): Remove <channel/server>InterceptorFactory and replace with respective
// interceptors
private void setProducer(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -196,9 +197,9 @@ 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);
List<?> configurators = InternalConfiguratorRegistry.getConfigurators();
assertThat(configurators).hasSize(1);
ObservabilityConfigurator configurator = (ObservabilityConfigurator) configurators.get(0);
Expand All @@ -208,6 +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, 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) {
Copy link
Member

Choose a reason for hiding this comment

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

When the try block is exited, it calls close() again. We should be seeing a IllegalStateException exception. If the tests are passing, that needs to be looked in to.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

Copy link
Member

Choose a reason for hiding this comment

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

No, I wasn't asking for it to be closed yet again. Remove that part added. It seems the answer to my comment is try-with-resources isn't being used here. I wonder if the diff folding made me think it was.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, try-with-resource block is replaced with a classic try-catch block and explicitly invoked with a new method closeWithSleepTime of 3sec.

fail("Encountered exception: " + e);
}
Expand Down Expand Up @@ -261,4 +269,25 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
return next.startCall(call, headers);
}
}

// Test to verify the closeWithSleepTime method along with sleep time explicitly.
@Test
public void closeWithSleepTime() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

This seems identical to StaticTestingClassEnableObservability, except for the verify(sink).close(). That would be good to move to the other test and then we delete this one.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

Copy link
Member

Choose a reason for hiding this comment

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

You deleted the verify(sink).close() and left the test? That's the exact opposite of what I had asked for. What does this test that isn't part of StaticTestingClassEnableObservability?

Copy link
Author

Choose a reason for hiding this comment

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

Resolved.

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);
}
}