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.

Sign up for GitHub

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

Jump to bottom

gcp-observability: Optimize GcpObservabilityTest.enableObservability execution time #11783

Merged
merged 11 commits into from
Jan 24, 2025
Merged

gcp-observability: Optimize GcpObservabilityTest.enableObservability execution time #11783

merged 11 commits into from
Jan 24, 2025

Conversation

harshagoo94
Copy link
Contributor

@harshagoo94 harshagoo94 commented Dec 26, 2024 •
edited
Loading

gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case.

Fixes #10146.

Snippet before fix as execuation time taking 1m 21sec

10146-before-fix

Snippet after fix as execuation time taking below 1sec 44msec.

10146-after-fix

@harshagoo94 harshagoo94 marked this pull request as ready for review December 26, 2024 07:54
@shivaspeaks shivaspeaks added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 26, 2024
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Dec 26, 2024
shivaspeaks
shivaspeaks reviewed Dec 26, 2024
View reviewed changes
Copy link
Member

@shivaspeaks shivaspeaks left a comment

Choose a reason for hiding this comment

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

If this is the problem then should we do same in line 233 in original code or 236 after your commit as well? It has the same try-with-resource block usage and I don't see anywhere in grpc-java anything similar like this.

@shivaspeaks shivaspeaks requested a review from ejona86 December 26, 2024 09:42
@harshagoo94
Copy link
Contributor Author

If this is the problem then should we do same in line 233 in original code or 236 after your commit as well? It has the same try-with-resource block usage and I don't see anywhere in grpc-java anything similar like this.

@shivaspeaks ,

As I have observed, enabling observability requires initializing certain things and classes, which makes the task within the try-with-resources block take longer. However, when observability is disabled, I did not observe many initializations in the test cases at lines 233 and 236, so I refactored the code to use a traditional try-catch block instead.

@ejona86
Copy link
Member

ejona86 commented Dec 26, 2024

You just don't close? No, we need to clean up and we want to test cleanup. Without close() we will leak resources and probably threads, and we don't want those running during other tests.

@harshagoo94
Copy link
Contributor Author

You just don't close? No, we need to clean up and we want to test cleanup. Without close() we will leak resources and probably threads, and we don't want those running during other tests.

@ejona86,

I’m working on this and plan to implement a close() method with a configurable sleep time. After the test finishes, the close() method will be called with a shorter sleep duration to speed up the cleanup process.

@ejona86
Copy link
Member

ejona86 commented Dec 27, 2024

That sounds fine; just make the new method package-private.

@harshagoo94
Copy link
Contributor Author

@shivaspeaks ,

As per the suggestions above, I have pushed the latest changes, which include the addition of a new close method with sleep time. After executing the test case, I found that the execution time is now under 5 seconds.

Could you please review the changes.

@harshagoo94
gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest…
d8e6741 
….enableObservability test case - fixed review comments.
@harshagoo94
gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest…
f0d4344 
….enableObservability test case - fixed review comments.
vinodhabib
vinodhabib reviewed Jan 3, 2025
View reviewed changes
Copy link
Contributor

@vinodhabib vinodhabib left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Changes LGTM and Lets wait for Review from @ejona86 @shivaspeaks

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 3, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 3, 2025
@ejona86 ejona86 changed the title gcp-obsrevability : Fixed the slowness issue for GcpObservabilityTest.enableObservability test case. gcp-observability: Fixed slowness for GcpObservabilityTest.enableObservability test case Jan 3, 2025
@harshagoo94
Copy link
Contributor Author

@ejona86 @shivaspeaks

I have resolved all comments as per above mentioned and Kindly look into it once.

ejona86
ejona86 approved these changes Jan 22, 2025
View reviewed changes
gcp-observability/src/main/java/io/grpc/gcp/observability/GcpObservability.java
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
Copy link
Member

Choose a reason for hiding this comment

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

Restore this comment. If you don't think it makes sense here any more (I'm assuming that's why you deleted it), then at least move it to close() just before the closeWithSleepTime() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved and restored the comment.

@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 22, 2025
@grpc-kokoro grpc-kokoro removed kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run labels Jan 22, 2025
@shivaspeaks shivaspeaks changed the title gcp-observability: Fixed slowness for GcpObservabilityTest.enableObservability test case gcp-observability: Reduce GcpObservabilityTest.enableObservability execution time Jan 23, 2025
shivaspeaks
shivaspeaks reviewed Jan 23, 2025
View reviewed changes
gcp-observability/src/test/java/io/grpc/gcp/observability/GcpObservabilityTest.java Outdated
@@ -49,6 +49,7 @@
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
import java.util.concurrent.TimeUnit;
Copy link
Member

Choose a reason for hiding this comment

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

Please put it in Lexicographical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved

@harshagoo94
Copy link
Contributor Author

@shivaspeaks, I have resolved all the comments and all checks have passed. Kindly look into it once.

@shivaspeaks shivaspeaks added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 24, 2025
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Jan 24, 2025
@shivaspeaks shivaspeaks changed the title gcp-observability: Reduce GcpObservabilityTest.enableObservability execution time gcp-observability: Optimize GcpObservabilityTest.enableObservability execution time Jan 24, 2025
@shivaspeaks shivaspeaks merged commit 67351c0 into grpc:master Jan 24, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivaspeaks shivaspeaks shivaspeaks approved these changes

@vinodhabib vinodhabib vinodhabib left review comments

@ejona86 ejona86 ejona86 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GcpObservabilityTest.enableObservability is way slow
5 participants
@harshagoo94 @ejona86 @vinodhabib @shivaspeaks @grpc-kokoro