-
Notifications
You must be signed in to change notification settings - Fork 69
feat: Add Metrics feature flag #4044
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
Conversation
Summary of ChangesHello @diegomarquezp, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements a new feature flag to control the activation of metrics tracing within the GAX library. This enhancement allows developers to easily enable or disable metrics collection at runtime using system properties or environment variables, providing more flexibility and control over application performance monitoring. The changes include a dedicated utility class for flag management, modifications to the metrics tracer factory, and thorough testing to ensure robust and predictable behavior. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a feature flag, GOOGLE_CLOUD_ENABLE_METRICS, to control metrics collection. The implementation in MetricsUtils correctly checks for a system property or environment variable. The changes are well-tested, including unit tests for the new utility class and integration tests for the feature flag behavior. My review includes suggestions to improve test code style by using explicit imports and to enhance the robustness of a test case by avoiding the use of a null MetricsRecorder.
java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java
Outdated
Show resolved
Hide resolved
java-showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITOtelMetrics.java
Outdated
Show resolved
Hide resolved
…/v1beta1/it/ITOtelMetrics.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…leapis/sdk-platform-java into observability/feature-flags
|
|
|
|
||
| @Override | ||
| public ApiTracer newTracer(ApiTracer parent, SpanName spanName, OperationType operationType) { | ||
| if (!MetricsUtils.isMetricsEnabled()) { |
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.
I don't want to change how existing metrics works, the flag is intended to protect new metric instrumentation from people taking dependencies on it while it stabilizes.
westarle
left a comment
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.
I like the API and the implementation, but we shouldn't apply it to existing metrics. I also think the first one we might be interested in is for tracing, if we are going to work on tracing first.
/cc @blakeli0
| * system property. | ||
| */ | ||
| public static boolean isMetricsEnabled() { | ||
| String enableMetrics = System.getProperty(GOOGLE_CLOUD_ENABLE_METRICS); |
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.
I would like this to have the words "JAVA" and "EXPERIMENTAL" or "UNSTABLE" in it. The Go one is GOOGLE_SDK_GO_EXPERIMENTAL_FOO
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.
The "JAVA" part of this comment is intended to apply to the env variable rather than the Java property...
| public static boolean isMetricsEnabled() { | ||
| String enableMetrics = System.getProperty(GOOGLE_CLOUD_ENABLE_METRICS); | ||
| if (enableMetrics == null) { | ||
| enableMetrics = System.getenv(GOOGLE_CLOUD_ENABLE_METRICS); |
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.
Do we need to cache the result, or can we put a note on the method that it should be cached by clients (I'd like to avoid a read to env or properties on every RPC).
|
I will close this for now and keep the feedback for the next iteration. |





This PR introduces a feature flag to control the collection of metrics in GAX. Metrics are now disabled by default and can be enabled via an environment variable or system property.
Changes:
MetricsUtils.javato handle the feature flag logic. It checks for theGOOGLE_CLOUD_ENABLE_METRICSenvironment variable or system property.MetricsTracerFactory.javato respect the feature flag. If metrics are disabled,newTracernow returns aBaseApiTracer(no-op) instead of aMetricsTracer.MetricsTracer.javato log anINFOmessage once per JVM instance if it is instantiated while metrics are disabled. This helps users who have explicitly configured aMetricsTracerFactorydiscover that they also need to enable the global feature flag.MetricsUtilsTest.javato verify the feature flag logic. UpdatedMetricsTracerFactoryTest.javato ensure it correctly returns a no-op tracer when metrics are disabled.ITOtelMetrics.javain the showcase module to include a new test casetestMetricsFeatureFlag(), which verifies the end-to-end behavior of the flag.MetricsTracerFactoryTest.javausing@ExtendWith(MockitoExtension.class).How to enable metrics:
To enable metrics collection, set the following environment variable or system property to true:
GOOGLE_CLOUD_ENABLE_METRICS=true