-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
base: master
Are you sure you want to change the base?
Changes from 5 commits
d581169
8311f00
d8e6741
f0d4344
e7289e5
3d2e3ba
f481f92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
if (config.isEnableCloudMonitoring() || config.isEnableCloudTracing()) { | ||
try { | ||
long sleepTimeMillis = TimeUnit.MILLISECONDS.convert(sleepTime, timeUnit); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
|
@@ -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); | ||
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems identical to StaticTestingClassEnableObservability, except for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You deleted the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).