-
Notifications
You must be signed in to change notification settings - Fork 9.3k
feat(config): Support telemetry configuration via environment variables #9113
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 @jerop, 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 introduces a significant enhancement to the telemetry configuration system, allowing users to define telemetry settings through environment variables. This provides greater flexibility and dynamic control, especially in containerized or automated environments. The new system ensures that environment variables override settings from 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
|
|
Size Change: +2.2 kB (+0.01%) Total Size: 17.4 MB
ℹ️ View Unchanged
|
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 well-structured approach to configuring telemetry settings via environment variables, which is a great enhancement. The logic is cleanly refactored into a new resolveTelemetrySettings function in the core package, with a clear precedence order of CLI flags, environment variables, and then settings files. The changes are accompanied by thorough unit tests and documentation updates. I've found one issue in the test suite where a test case asserts incorrect behavior, which should be corrected to ensure the tests accurately reflect the implementation's fail-fast validation.
b4e7aac to
fe4f940
Compare
|
/gemini review |
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 is a great improvement. It successfully introduces support for configuring telemetry via environment variables, which was a requested feature. The changes are well-structured, moving the configuration resolution logic into the core package, which improves modularity and maintainability. The addition of comprehensive unit and integration tests ensures the new functionality is reliable and that the configuration precedence is handled correctly. The documentation updates are clear and thorough, making it easy for users to understand the new environment variables and the overall configuration hierarchy. The code is clean, and the implementation is solid. I have no critical or high-severity issues to report.
fe4f940 to
4e7e104
Compare
This change introduces support for configuring telemetry settings using environment variables, as requested in issue #9084. The following environment variables are now supported and will take precedence over their corresponding values in `settings.json`: - GEMINI_TELEMETRY_ENABLED - GEMINI_TELEMETRY_TARGET - GEMINI_TELEMETRY_OTLP_ENDPOINT - GEMINI_TELEMETRY_OTLP_PROTOCOL - GEMINI_TELEMETRY_LOG_PROMPTS - GEMINI_TELEMETRY_OUTFILE - GEMINI_TELEMETRY_USE_COLLECTOR A regression test suite has been added to verify that the environment variables are correctly prioritized and that the system correctly falls back to using values from `settings.json` when the environment variables are not set. The documentation in `docs/telemetry.md` and `docs/cli/configuration.md` has been updated to reflect these new environment variables and the configuration hierarchy. fixes #9084
4e7e104 to
9302c8c
Compare
… via environment variables (google-gemini#9113)
This change introduces support for configuring telemetry settings using environment variables, as requested in issue #9084.
The following environment variables are now supported and will take precedence over their corresponding values in
settings.json.A regression test suite has been added to verify that the environment variables are correctly prioritized and that the system correctly falls back to using values from
settings.jsonwhen the environment variables are not set.The documentation in
docs/telemetry.mdanddocs/cli/configuration.mdhas been updated to reflect these new environment variables and the configuration hierarchy.Fixes #9084
enabledGEMINI_TELEMETRY_ENABLED--telemetry/--no-telemetrytrue/falsefalsetargetGEMINI_TELEMETRY_TARGET--telemetry-target <local|gcp>"gcp"/"local""local"otlpEndpointGEMINI_TELEMETRY_OTLP_ENDPOINT--telemetry-otlp-endpoint <URL>http://localhost:4317otlpProtocolGEMINI_TELEMETRY_OTLP_PROTOCOL--telemetry-otlp-protocol <grpc|http>"grpc"/"http""grpc"outfileGEMINI_TELEMETRY_OUTFILE--telemetry-outfile <path>otlpEndpoint: "")logPromptsGEMINI_TELEMETRY_LOG_PROMPTS--telemetry-log-prompts/--no-telemetry-log-promptstrue/falsetrueuseCollectorGEMINI_TELEMETRY_USE_COLLECTORtrue/falsefalse