-
Notifications
You must be signed in to change notification settings - Fork 100
feat(spanner): make built-in metrics enabled by default #1459
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 @sinhasubham, 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 significantly enhances the out-of-the-box observability for Spanner users by enabling built-in OpenTelemetry metrics by default. The change shifts the control mechanism from an opt-in model to an opt-out model, simplifying metric collection for most users while still providing a clear way to disable them if needed. Highlights
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 correctly inverts the logic for enabling built-in metrics, making them on by default, which is a great improvement for out-of-the-box observability. The changes to the environment variable and the corresponding updates in the client logic and unit tests are clear and well-executed. I've noted one minor point about a leftover constant that could be cleaned up to improve maintainability.
|
|
||
| def _get_spanner_enable_builtin_metrics(): | ||
| return os.getenv(ENABLE_SPANNER_METRICS_ENV_VAR) == "true" | ||
| return os.getenv(SPANNER_DISABLE_BUILTIN_METRICS_ENV_VAR) != "true" |
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.
Since we are making Metrics default enabled with this changes, please make sure that metrics are not enabled when emulator is enabled and when customers use NoCredentials
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.
yes these cases are getting handled.
Emulator condition: https://github.com/googleapis/python-spanner/blob/main/google/cloud/spanner_v1/client.py#L257
NoCredential user condition: https://github.com/googleapis/python-spanner/blob/main/google/cloud/spanner_v1/client.py#L224-L225
This sets emulator setting for NoCredentials users making them act like emulator which means disabled metrics.
|
Please see if we can also add Integration test for these metrics like https://github.com/googleapis/java-spanner/blob/main/google-cloud-spanner/src/test/java/com/google/cloud/spanner/it/ITBuiltInMetricsTest.java#L89 |
28abdfd to
b13ea7b
Compare
b13ea7b to
63ca819
Compare
|
Got a confirmation from @surbhigarg92 that unexpected enabling of metrics for customers who did not set ENABLE_SPANNER_METRICS_ENV_VAR previously is not a breaking change and can be merged. Merging this PR. |
Make built-in metrics enabled by default
This change inverts the logic for enabling built-in OpenTelemetry metrics. Previously, metrics were disabled by default and could be enabled by setting
ENABLE_SPANNER_METRICS_ENV_VAR=true.With this update, metrics are now enabled by default to provide better out-of-the-box observability for users.
To disable metrics, users must now set the new environment variable:
SPANNER_DISABLE_BUILTIN_METRICS=trueThe old
ENABLE_SPANNER_METRICS_ENV_VARis no longer used. Unit tests have been updated to reflect this new opt-out behavior.BREAKING CHANGE: Built-in metrics are now enabled by default. Users who previously did not set any environment variables will have metrics collection and export turned on automatically after upgrading. To restore the previous behavior and disable metrics, thry have to set the
SPANNER_DISABLE_BUILTIN_METRICSenvironment variable totrue.