[Feature] [Platform] Allows to opt out in the Inventory Telemetry #1980
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on November 28. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds the ability for users to opt out of telemetry collection when generating license inventories. A new --telemetry flag is introduced (enabled by default) that allows users to control whether telemetry data is collected during inventory operations.
- Added a new
--telemetryflag to the license inventory and activate commands - Introduced a
Configurationstruct to pass telemetry settings through the inventory fetching pipeline - Refactored AQL execution functions to support telemetry-conditional execution
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/platform/flags.go | Defines the new flagTelemetry boolean flag with default value true |
| pkg/platform/license_inventory.go | Registers the telemetry flag for the inventory command |
| pkg/platform/license_activate.go | Registers the telemetry flag for the activate command |
| pkg/platform/license.go | Reads the telemetry flag value and passes it through the configuration |
| pkg/platform/inventory/configuration.go | New file defining the Configuration struct with telemetry settings |
| pkg/platform/inventory/inventory.go | Updates function signatures to accept and propagate the Configuration |
| pkg/platform/inventory/aql.go | Refactors AQL execution to support telemetry-conditional logic |
| pkg/platform/inventory/fetcher.*.go | Updates fetcher function signatures to accept Configuration parameter |
| docs/cli/arangodb_operator_platform.md | Documents the new telemetry flag in CLI documentation |
| CHANGELOG.md | Records the new feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pkg/platform/license.go
Outdated
|
|
||
| var cfg inventory.Configuration | ||
|
|
||
| if v, err := flagTelemetry.Get(cmd); !v || err != nil { |
There was a problem hiding this comment.
The logic incorrectly sets telemetry to false when the flag value is true OR when there's an error. The condition !v || err != nil means telemetry will be disabled if the flag is true (since !true evaluates to false, but the intent seems inverted). Additionally, if there's an error reading the flag, it silently defaults to disabled. The correct logic should be if err != nil || !v { cfg.Telemetry = util.NewType(false) } to handle the error case first and then check if telemetry is explicitly disabled.
| if v, err := flagTelemetry.Get(cmd); !v || err != nil { | |
| if v, err := flagTelemetry.Get(cmd); err != nil || !v { |
| log.Info("Telemetry disabled") | ||
| return nil | ||
| } else { | ||
| log.Info("Collecting Telemetry details") |
There was a problem hiding this comment.
The log message 'Collecting Telemetry details' is misleading because it's shown for all AQL queries, not just telemetry-related ones. When telemetry=false (for basic AQL queries), this message still appears. Consider moving the logging inside the conditional branches or making the message more accurate, e.g., 'Executing AQL query' for non-telemetry queries.
| log.Info("Collecting Telemetry details") | |
| if telemetry { | |
| log.Info("Collecting Telemetry details") | |
| } else { | |
| log.Info("Executing AQL query") | |
| } |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if v, err := flagTelemetry.Get(cmd); err != nil || !v { | ||
| cfg.Telemetry = util.NewType(false) | ||
| } |
There was a problem hiding this comment.
The logic for setting cfg.Telemetry is inverted and unclear. When telemetry is enabled (v == true), the field remains nil, but when disabled (!v) or on error, it's explicitly set to false. This makes the nil state ambiguous. Consider always setting the value explicitly: cfg.Telemetry = util.NewType(v) when no error occurs, or handle the error case separately.
| if v, err := flagTelemetry.Get(cmd); err != nil || !v { | |
| cfg.Telemetry = util.NewType(false) | |
| } | |
| v, err := flagTelemetry.Get(cmd) | |
| if err != nil { | |
| return nil, err | |
| } | |
| cfg.Telemetry = util.NewType(v) |
| func (c *Configuration) WithTelemetry() bool { | ||
| if c == nil || c.Telemetry == nil { | ||
| return true | ||
| } | ||
| return *c.Telemetry | ||
| } |
There was a problem hiding this comment.
The WithTelemetry method lacks a docstring explaining that it returns true by default when configuration is nil or telemetry is unset. This default behavior is important for understanding the opt-out nature of the feature and should be documented.
No description provided.