[Telemetry] Refactor getModules method to use cached standard modules from firestore#5570
Conversation
Summary of ChangesHello, 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 refactors the telemetry module retrieval process to rely on cached data from Firestore rather than performing live network requests to the GitHub API. This change improves the reliability and speed of the telemetry collection process by reducing external dependencies and leveraging pre-cached release metadata. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request replaces the GitHub-based module discovery logic with a Firestore-backed approach, updating the telemetry collector to fetch standard module metadata from a database instead of the Git tree. Feedback highlights the need to restore test coverage for telemetry sanitization and suggests optimizing the metadata loading process to avoid redundant network requests and improve testability.
I am having trouble creating individual review comments. Click here to see my feedback.
pkg/telemetry/collector_test.go (1037-1126)
The removal of TestGetModules eliminates the test coverage for the telemetry sanitization logic. This logic is critical for ensuring that user privacy is maintained by masking custom module paths with the "Custom" label. While the previous implementation relied on mocking HTTP requests to GitHub, the test should be restored and updated to mock the getStandardModules function (provided it is refactored to be a variable as suggested).
pkg/telemetry/collector_util.go (92-98)
The getStandardModules function currently triggers a network request to Firestore via loadReleaseMetadata on every call. This is inefficient. Consider checking if the data is already present in viper before calling loadReleaseMetadata, and refactor the function into a variable to allow mocking in tests. Note that as a package-level variable used for mocking, it must include a comment explaining its purpose.
// getStandardModules is a package-level variable to allow mocking in unit tests.
var getStandardModules = func() []string {
if viper.IsSet("standard_modules") {
return viper.GetStringSlice("standard_modules")
}
if err := loadReleaseMetadata(); err != nil {
return []string{}
}
return viper.GetStringSlice("standard_modules")
}References
- When adding a package-level variable, ensure it has a brief comment explaining its purpose, especially if it's for mocking in unit tests, to adhere to documentation standards.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request replaces the GitHub-based module discovery mechanism with a Firestore-backed metadata cache. It removes the GetPredefinedModules function from pkg/config and introduces loadReleaseMetadata in pkg/telemetry, which fetches and stores standard module lists in the global Viper registry. Feedback was provided regarding the getStandardModules implementation to avoid redundant network calls by checking the cache before triggering a fetch.
Repository rule violations found
2f352a0
into
GoogleCloudPlatform:develop
This pull request refactors the telemetry module retrieval process to rely on cached data from Firestore for getting pre-defined (standard) modules rather than performing live network requests to the GitHub API. This change improves the reliability and speed of the telemetry collection process by reducing external dependencies and leveraging pre-cached release metadata.
Changes: