usernaut opentelemetry shared package along with Reconciliation metrics implementation" - #172
usernaut opentelemetry shared package along with Reconciliation metrics implementation" #172pratyush-anand1444 wants to merge 1 commit into
Conversation
Summary of ChangesHello @pratyush-anand1444, 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 upgrades the Usernaut operator by integrating OpenTelemetry for enhanced observability and introducing a new HTTP API server for external interactions. It expands the operator's capabilities with a new Snowflake client, allowing for management of users and teams in Snowflake. The core group reconciliation logic has been made more robust, supporting nested groups and improving status reporting. Additionally, the development workflow is streamlined with new guidelines and pre-commit hooks to ensure code quality and security. 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. Ignored Files
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. Go code flows, clean and fast, Kubernetes, a new task. Metrics rise, a watchful eye, Snowflake joins, beneath the sky. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements, including a new OpenTelemetry package for metrics, a Snowflake client, and an HTTP API server. The controller logic has been substantially improved to support nested groups, finalizers for cleanup, and more robust status reporting. Additionally, the build and deployment processes have been refactored using Kustomize overlays, and client implementations have been optimized for batch operations and concurrency. My review focuses on potential improvements in configuration handling, error reporting, and efficiency in the new and updated components.
| .PHONY: manifests | ||
| manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. | ||
| $(CONTROLLER_GEN) rbac:roleName=manager-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases | ||
| $(CONTROLLER_GEN) rbac:roleName=manager-role crd:allowDangerousTypes=true webhook paths="./..." output:crd:artifacts:config=config/crd/bases |
There was a problem hiding this comment.
The crd:allowDangerousTypes=true flag has been added. This can be risky as it bypasses some of the type safety checks for CRD generation, potentially allowing for fields that can hold arbitrary data (like runtime.RawExtension). While the current changes to the Group CRD seem safe, please double-check if this flag is strictly necessary. If it's not, it would be safer to remove it to enforce stricter CRD validation and enhance security.
| func (c *SnowflakeClient) AddUserToTeam(ctx context.Context, teamID string, userIDs []string) error { | ||
| log := logger.Logger(ctx).WithFields(logrus.Fields{ | ||
| "service": "snowflake", | ||
| "teamID": teamID, | ||
| "user_count": len(userIDs), | ||
| }) | ||
| log.Info("adding users to team") | ||
|
|
||
| for _, userID := range userIDs { | ||
| endpoint := fmt.Sprintf("/api/v2/users/%s/grants", userID) | ||
|
|
||
| resp, status, err := c.makeRoleRequest(ctx, teamID, endpoint) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to add user %s to team %s: %w", userID, teamID, err) | ||
| } | ||
|
|
||
| if status != http.StatusOK && status != http.StatusCreated { | ||
| return fmt.Errorf("failed to add user %s to team %s, status: %s, body: %s", | ||
| userID, teamID, http.StatusText(status), string(resp)) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // RemoveUserFromTeam removes users from a team (revokes role from users) | ||
| func (c *SnowflakeClient) RemoveUserFromTeam(ctx context.Context, teamID string, userIDs []string) error { | ||
| log := logger.Logger(ctx).WithFields(logrus.Fields{ | ||
| "service": "snowflake", | ||
| "teamID": teamID, | ||
| "user_count": len(userIDs), | ||
| }) | ||
| log.Info("removing users from team") | ||
|
|
||
| for _, userID := range userIDs { | ||
| endpoint := fmt.Sprintf("/api/v2/users/%s/grants:revoke", userID) | ||
|
|
||
| resp, status, err := c.makeRoleRequest(ctx, teamID, endpoint) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to remove user %s from team %s: %w", userID, teamID, err) | ||
| } | ||
|
|
||
| if status != http.StatusOK && status != http.StatusNoContent { | ||
| return fmt.Errorf("failed to remove user %s from team %s, status: %s, body: %s", | ||
| userID, teamID, http.StatusText(status), string(resp)) | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
The implementations for AddUserToTeam and RemoveUserFromTeam iterate through user IDs and make an individual API call for each user. This approach can be inefficient and may lead to API rate limiting, especially for groups with many members.
It would be beneficial to investigate if the Snowflake REST API provides endpoints for granting or revoking roles for multiple users in a single batch request. If such endpoints exist, refactoring these functions to use batch operations would significantly improve performance and resilience.
| - [ ] Updates client factory in `pkg/clients/client.go` | ||
| - [ ] Dependencies added to `go.mod` with appropriate versions | ||
| - [ ] No unnecessary custom HTTP clients created | ||
| - [ ] Ensure `go mod vendor` is executed and `vendor/` directory is upto date |
There was a problem hiding this comment.
Minor typo correction for better readability.
| - [ ] Ensure `go mod vendor` is executed and `vendor/` directory is upto date | |
| - [ ] Ensure `go mod vendor` is executed and `vendor/` directory is up to date |
|
|
||
| if err := telemetry.Init(ctx, telemetryConfig); err != nil { | ||
| setupLog.Error(err, "failed to initialize telemetry") | ||
| // we need to continue without telemntry so that reconciliation doesn't stop |
There was a problem hiding this comment.
There's a typo in the comment. It should be telemetry.
// we need to continue without telemetry so that reconciliation doesn't stop| if err := r.Cache.Delete(ctx, transformed_group_name); err != nil { | ||
| backendLoggerInfo.WithError(err).Error("Finalizer: failed to delete cache entry after cleanup") | ||
| return err | ||
| } |
There was a problem hiding this comment.
The call to r.Cache.Delete here is unnecessary. The logic already modifies the teamDetailsMap in memory by removing the relevant backend's entry. The subsequent block then overwrites the cache entry with the updated map using r.Cache.Set if the map is not empty. Deleting the key first is redundant. Removing this block will make the finalizer logic slightly more efficient.
| if len(allErrors) > 0 { | ||
| return fmt.Errorf("multiple errors occurred: %v", allErrors) | ||
| } |
There was a problem hiding this comment.
Since you are on Go 1.24, you can use errors.Join to wrap multiple errors. This is more idiomatic than formatting them into a string and allows callers to programmatically inspect the wrapped errors. This also applies to the RemoveUserFromTeam function. You'll need to import the errors package.
| if len(allErrors) > 0 { | |
| return fmt.Errorf("multiple errors occurred: %v", allErrors) | |
| } | |
| if len(allErrors) > 0 { | |
| return errors.Join(allErrors...) | |
| } |
| if len(allErrors) > 0 { | ||
| return fmt.Errorf("multiple errors occurred: %v", allErrors) | ||
| } |
There was a problem hiding this comment.
Similar to AddUserToTeam, consider using errors.Join here to provide a more structured way of returning multiple errors from concurrent operations. You'll need to import the errors package.
| if len(allErrors) > 0 { | |
| return fmt.Errorf("multiple errors occurred: %v", allErrors) | |
| } | |
| if len(allErrors) > 0 { | |
| return errors.Join(allErrors...) | |
| } |
acbeacd to
d546c91
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry for metrics, which is a great addition for observability. It adds a new telemetry package with helpers and wrappers for OTel metrics, and integrates it into main.go for initialization and into group_controller.go for recording reconciliation metrics. The overall structure of the telemetry package is well-designed. However, there are a few issues to address: a hardcoded telemetry endpoint, an incomplete Gauge metric implementation, and some confusing logic in main.go that could be simplified. I've provided specific comments and suggestions for these points. Addressing them will make this a very solid contribution.
| type Gauge struct { | ||
| gauge otelmetric.Float64ObservableGauge | ||
| } | ||
|
|
||
| func NewGauge(meter otelmetric.Meter, opts MetricOptions) (*Gauge, error) { | ||
| gauge, err := meter.Float64ObservableGauge( | ||
| opts.Name, | ||
| otelmetric.WithDescription(opts.Description), | ||
| otelmetric.WithUnit(opts.Unit), | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &Gauge{gauge: gauge}, nil | ||
| } |
There was a problem hiding this comment.
The implementation for Gauge is incomplete. otelmetric.Float64ObservableGauge requires a callback function to be registered at creation time to observe and report values. Without a callback, this gauge will never report any data. The NewGauge function should be updated to accept a callback function.
Here is an example from the OpenTelemetry documentation:
_, err = meter.Float64ObservableGauge(
"heap.size",
metric.WithFloat64Callback(func(_ context.Context, o metric.Float64Observer) error {
o.Observe(heapSize)
return nil
}),
)Since this Gauge wrapper is not used in the PR, you could also consider removing it for now if it's not needed, to avoid shipping incomplete code.
| if err := telemetry.Init(ctx, telemetryConfig); err != nil { | ||
| setupLog.Error(err, "failed to initialize telemetry") | ||
| // we need to continue without telemntry so that reconciliation doesn't stop | ||
| } else { | ||
| if telemetryConfig.Enabled { | ||
| setupLog.Info("telemetry initialized successfully") | ||
| meter := telemetry.GetMeter("usernaut") | ||
| if err := telemetry.InitReconciliationMetrics(meter); err != nil { | ||
| setupLog.Error(err, "failed to initialize reconciliation metrics") | ||
| } else { | ||
| setupLog.Info("reconciliation metrics initialized successfully") | ||
| } | ||
| defer func() { | ||
| if err := telemetry.Shutdown(ctx); err != nil { | ||
| setupLog.Error(err, "failed to shutdown telemetry") | ||
| } | ||
| }() | ||
| } else { | ||
| setupLog.Info("telemetry is disabled, skipping initialization") | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for initializing telemetry and handling the disabled case can be simplified for better readability. The telemetry.Init function already handles the case where telemetry is disabled by setting up a no-op provider. The current structure with a nested if/else and the log message "skipping initialization" is a bit misleading since Init has already been called. Consider refactoring this block to be more straightforward.
if err := telemetry.Init(ctx, telemetryConfig); err != nil {
setupLog.Error(err, "failed to initialize telemetry, continuing without it")
} else if telemetryConfig.Enabled {
setupLog.Info("telemetry initialized successfully")
meter := telemetry.GetMeter("usernaut")
if err := telemetry.InitReconciliationMetrics(meter); err != nil {
setupLog.Error(err, "failed to initialize reconciliation metrics")
} else {
setupLog.Info("reconciliation metrics initialized successfully")
}
defer func() {
if err := telemetry.Shutdown(ctx); err != nil {
setupLog.Error(err, "failed to shutdown telemetry")
}
}()
} else {
setupLog.Info("telemetry is disabled")
}|
|
||
| if err := telemetry.Init(ctx, telemetryConfig); err != nil { | ||
| setupLog.Error(err, "failed to initialize telemetry") | ||
| // we need to continue without telemntry so that reconciliation doesn't stop |
There was a problem hiding this comment.
There's a typo in the comment. It should be 'telemetry'.
| // we need to continue without telemntry so that reconciliation doesn't stop | |
| // we need to continue without telemetry so that reconciliation doesn't stop |
| func WithAttributes(attrs ...attribute.KeyValue) []attribute.KeyValue { | ||
| return attrs | ||
| } |
There was a problem hiding this comment.
The WithAttributes function is redundant. It takes a variadic attribute.KeyValue and returns a slice of attribute.KeyValue, which is what Go does automatically for variadic parameters. This function doesn't add any value and can be removed to simplify the API of this package.
ac73212 to
5cc1892
Compare
…cs implementation Signed-off-by: Pratyush Anand <praanand@redhat.com>
5cc1892 to
7ac06e7
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces OpenTelemetry-based observability infrastructure for Usernaut, specifically implementing reconciliation metrics to track controller performance. The changes include a shared telemetry package with metric abstractions, configuration support, and integration into the Group controller.
Changes:
- Added comprehensive OpenTelemetry metrics package with Counter, Histogram, Gauge, and UpDownCounter wrappers
- Implemented reconciliation-specific metrics tracking for monitoring controller success/error rates
- Integrated telemetry initialization and shutdown lifecycle management in the main application entry point
Reviewed changes
Copilot reviewed 11 out of 320 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/telemetry/telemetry.go | Core telemetry initialization with OTLP HTTP exporter setup and meter provider management |
| pkg/telemetry/reconciliation.go | Reconciliation-specific metrics for tracking controller attempts and errors |
| pkg/telemetry/metrics.go | Generic metric type wrappers (Counter, Histogram, Gauge, UpDownCounter) for OpenTelemetry |
| pkg/telemetry/helpers.go | Metric naming conventions and attribute helper functions |
| pkg/config/telemetry_config.go | Configuration struct for telemetry settings |
| pkg/config/loader.go | Refactored config loading to properly merge default and environment-specific configs |
| pkg/config/config.go | Added TelemetryConfig field to AppConfig |
| internal/controller/group_controller.go | Integrated reconciliation metrics recording into Group controller |
| go.mod | Updated OpenTelemetry dependencies to v1.39.0 |
| cmd/main.go | Added telemetry initialization and shutdown in application lifecycle |
| appconfig/default.yaml | Added default telemetry configuration |
| var initErr error | ||
| meterProviderOnce.Do(func() { | ||
| if !config.Enabled { | ||
| // no=op meter provider usage whenever telemetry is disabled |
There was a problem hiding this comment.
Corrected spelling of 'no=op' to 'no-op'.
| // no=op meter provider usage whenever telemetry is disabled | |
| // no-op meter provider usage whenever telemetry is disabled |
| OTLPEndpoint string | ||
| //default:false | ||
| Insecure bool | ||
| // default;true |
There was a problem hiding this comment.
Inconsistent comment format. Should use colon instead of semicolon to match line 49: 'default:true'.
| // default;true | |
| // default:true |
| // Configure viper settings ONCE upfront | ||
| c.viper.SetConfigType(c.opts.configType) | ||
| c.viper.AddConfigPath(c.opts.configPath) |
There was a problem hiding this comment.
The comment states settings are configured ONCE but these viper settings are reconfigured on every call to Load(). This could cause issues if Load() is called multiple times with different config paths. Consider moving these lines outside the Load() method or using sync.Once if this initialization should only happen once.
| // creates attribute for controller name | ||
| func WithController(controller string) attribute.KeyValue { | ||
| return attribute.String(AttrController, controller) | ||
| } | ||
|
|
||
| // creates attribute for backend name | ||
| func WithBackend(backend string) attribute.KeyValue { | ||
| return attribute.String(AttrBackend, backend) | ||
| } | ||
|
|
||
| // creates attribute for backend type | ||
| func WithBackendType(backendType string) attribute.KeyValue { | ||
| return attribute.String(AttrBackendType, backendType) | ||
| } | ||
|
|
||
| // creates attribute for status | ||
| func WithStatus(status string) attribute.KeyValue { | ||
| return attribute.String(AttrStatus, status) | ||
| } | ||
|
|
||
| // creates attribute for operation name | ||
| func WithOperation(operation string) attribute.KeyValue { | ||
| return attribute.String(AttrOperation, operation) | ||
| } | ||
|
|
||
| // creates attribute for error type |
There was a problem hiding this comment.
Comment should start with the function name per Go documentation conventions: 'WithController creates attribute for controller name'.
| // creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // creates attribute for error type | |
| // WithController creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // WithBackend creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // WithBackendType creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // WithStatus creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // WithOperation creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // WithError creates attribute for error type |
| // creates attribute for controller name | ||
| func WithController(controller string) attribute.KeyValue { | ||
| return attribute.String(AttrController, controller) | ||
| } | ||
|
|
||
| // creates attribute for backend name | ||
| func WithBackend(backend string) attribute.KeyValue { | ||
| return attribute.String(AttrBackend, backend) | ||
| } | ||
|
|
||
| // creates attribute for backend type | ||
| func WithBackendType(backendType string) attribute.KeyValue { | ||
| return attribute.String(AttrBackendType, backendType) | ||
| } | ||
|
|
||
| // creates attribute for status | ||
| func WithStatus(status string) attribute.KeyValue { | ||
| return attribute.String(AttrStatus, status) | ||
| } | ||
|
|
||
| // creates attribute for operation name | ||
| func WithOperation(operation string) attribute.KeyValue { | ||
| return attribute.String(AttrOperation, operation) | ||
| } | ||
|
|
||
| // creates attribute for error type |
There was a problem hiding this comment.
Comment should start with the function name per Go documentation conventions: 'WithBackend creates attribute for backend name'.
| // creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // creates attribute for error type | |
| // WithController creates attribute for controller name. | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // WithBackend creates attribute for backend name. | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // WithBackendType creates attribute for backend type. | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // WithStatus creates attribute for status. | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // WithOperation creates attribute for operation name. | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // WithError creates attribute for error type. |
| // creates attribute for controller name | ||
| func WithController(controller string) attribute.KeyValue { | ||
| return attribute.String(AttrController, controller) | ||
| } | ||
|
|
||
| // creates attribute for backend name | ||
| func WithBackend(backend string) attribute.KeyValue { | ||
| return attribute.String(AttrBackend, backend) | ||
| } | ||
|
|
||
| // creates attribute for backend type | ||
| func WithBackendType(backendType string) attribute.KeyValue { | ||
| return attribute.String(AttrBackendType, backendType) | ||
| } | ||
|
|
||
| // creates attribute for status | ||
| func WithStatus(status string) attribute.KeyValue { | ||
| return attribute.String(AttrStatus, status) | ||
| } | ||
|
|
||
| // creates attribute for operation name | ||
| func WithOperation(operation string) attribute.KeyValue { | ||
| return attribute.String(AttrOperation, operation) | ||
| } | ||
|
|
||
| // creates attribute for error type |
There was a problem hiding this comment.
Comment should start with the function name per Go documentation conventions: 'WithBackendType creates attribute for backend type'.
| // creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // creates attribute for error type | |
| // WithController creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // WithBackend creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // WithBackendType creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // WithStatus creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // WithOperation creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // WithError creates attribute for error type |
| return attribute.String(AttrBackendType, backendType) | ||
| } | ||
|
|
||
| // creates attribute for status |
There was a problem hiding this comment.
Comment should start with the function name per Go documentation conventions: 'WithStatus creates attribute for status'.
| // creates attribute for status | |
| // WithStatus creates attribute for status. |
| return attribute.String(AttrStatus, status) | ||
| } | ||
|
|
||
| // creates attribute for operation name |
There was a problem hiding this comment.
Extra space in comment. Should be 'creates attribute' instead of 'creates attribute'.
| // creates attribute for operation name | |
| // creates attribute for operation name |
| // creates attribute for controller name | ||
| func WithController(controller string) attribute.KeyValue { | ||
| return attribute.String(AttrController, controller) | ||
| } | ||
|
|
||
| // creates attribute for backend name | ||
| func WithBackend(backend string) attribute.KeyValue { | ||
| return attribute.String(AttrBackend, backend) | ||
| } | ||
|
|
||
| // creates attribute for backend type | ||
| func WithBackendType(backendType string) attribute.KeyValue { | ||
| return attribute.String(AttrBackendType, backendType) | ||
| } | ||
|
|
||
| // creates attribute for status | ||
| func WithStatus(status string) attribute.KeyValue { | ||
| return attribute.String(AttrStatus, status) | ||
| } | ||
|
|
||
| // creates attribute for operation name | ||
| func WithOperation(operation string) attribute.KeyValue { | ||
| return attribute.String(AttrOperation, operation) | ||
| } | ||
|
|
||
| // creates attribute for error type |
There was a problem hiding this comment.
Comment should start with the function name per Go documentation conventions: 'WithError creates attribute for error type'.
| // creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // creates attribute for error type | |
| // WithController creates attribute for controller name | |
| func WithController(controller string) attribute.KeyValue { | |
| return attribute.String(AttrController, controller) | |
| } | |
| // WithBackend creates attribute for backend name | |
| func WithBackend(backend string) attribute.KeyValue { | |
| return attribute.String(AttrBackend, backend) | |
| } | |
| // WithBackendType creates attribute for backend type | |
| func WithBackendType(backendType string) attribute.KeyValue { | |
| return attribute.String(AttrBackendType, backendType) | |
| } | |
| // WithStatus creates attribute for status | |
| func WithStatus(status string) attribute.KeyValue { | |
| return attribute.String(AttrStatus, status) | |
| } | |
| // WithOperation creates attribute for operation name | |
| func WithOperation(operation string) attribute.KeyValue { | |
| return attribute.String(AttrOperation, operation) | |
| } | |
| // WithError creates attribute for error type |
| // Validate telemetry configuration | ||
| if telemetryConfig.Enabled && telemetryConfig.OTLPEndpoint == "" { | ||
| setupLog.Error(fmt.Errorf("telemetry enabled but no endpoint configured"), | ||
| "Please set telemetry.otlp_endpoint in appconfig/local.yaml or via environment variable") | ||
| os.Exit(1) | ||
| } | ||
|
|
There was a problem hiding this comment.
This validation logic is duplicated - the same check is already performed inside telemetry.Init() at lines 64-70. The application will exit here before Init() can return its error, making the Init() validation unreachable. Remove this duplicate validation and rely on the error returned from telemetry.Init().
| // Validate telemetry configuration | |
| if telemetryConfig.Enabled && telemetryConfig.OTLPEndpoint == "" { | |
| setupLog.Error(fmt.Errorf("telemetry enabled but no endpoint configured"), | |
| "Please set telemetry.otlp_endpoint in appconfig/local.yaml or via environment variable") | |
| os.Exit(1) | |
| } |
There was a problem hiding this comment.
please address this comment.
this logic is present in https://github.com/redhat-data-and-ai/usernaut/pull/172/files#diff-1daf3b94af9e46763e64ff02c31240f3c3b1421994d4d56e05769dfd4f765ba4R68
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces OpenTelemetry support to the project, creating a shared telemetry package and implementing initial metrics for controller reconciliation. The changes are well-structured, adding configuration, initialization logic, and metric recording points in the group controller. My review focuses on ensuring the implementation aligns with the detailed OpenTelemetry guidelines in the project's style guide. I've identified opportunities to improve metric and attribute naming conventions, complete the set of recommended reconciliation metrics by adding duration tracking, and address a dependency version discrepancy. Overall, this is a great step towards better observability.
| go.opentelemetry.io/otel v1.39.0 | ||
| go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetrichttp v1.39.0 | ||
| go.opentelemetry.io/otel/metric v1.39.0 | ||
| go.opentelemetry.io/otel/sdk v1.39.0 | ||
| go.opentelemetry.io/otel/sdk/metric v1.39.0 |
There was a problem hiding this comment.
The OpenTelemetry dependencies have been added with version v1.39.0, but the project's style guide (line 1159) specifies v1.35.0. If this version bump is intentional, please consider updating the style guide to reflect the new standard for the project. If not, it would be best to align with the documented version to maintain consistency.
| func (r *GroupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (result ctrl.Result, err error) { | ||
| // reconciliation metrics recording using opentelemetry | ||
| reconciliationMetrics := telemetry.GetReconciliationMetrics() | ||
| if reconciliationMetrics != nil { | ||
| reconciliationMetrics.RecordReconciliationStart(ctx, controllerName) | ||
| defer func() { | ||
| if err != nil { | ||
| reconciliationMetrics.RecordReconciliationError(ctx, controllerName) | ||
| } | ||
| }() | ||
| } |
There was a problem hiding this comment.
It's great that you're adding reconciliation metrics. To make this even more valuable for performance monitoring, please consider also recording the reconciliation duration. The style guide (lines 1389-1404, 1549) recommends a histogram for this (usernaut.reconciliation.duration).
You could achieve this by capturing the start time at the beginning of the Reconcile function and recording the duration at the end within the defer block.
| type ReconciliationMetrics struct { | ||
| CountTotal *Counter | ||
| ErrorTotal *Counter | ||
| } |
There was a problem hiding this comment.
To fully align with the style guide's recommendations for observability (line 1549), please consider adding a duration histogram to the ReconciliationMetrics. This would allow tracking the performance of reconciliation loops. You would then need to initialize this histogram in InitReconciliationMetrics and record the duration in the controller.
| type ReconciliationMetrics struct { | |
| CountTotal *Counter | |
| ErrorTotal *Counter | |
| } | |
| type ReconciliationMetrics struct { | |
| CountTotal *Counter | |
| ErrorTotal *Counter | |
| Duration *Histogram | |
| } |
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
These extra blank lines can be removed for a more compact YAML file.
| } else { | ||
| if telemetryConfig.Enabled { | ||
| setupLog.Info("telemetry initialized successfully") | ||
| meter := telemetry.GetMeter("usernaut") |
There was a problem hiding this comment.
For consistency with the project's style guide (lines 1168, 1316), it would be better to use "usernaut/metrics" as the meter name. This helps in organizing and identifying metrics originating from this application.
| meter := telemetry.GetMeter("usernaut") | |
| meter := telemetry.GetMeter("usernaut/metrics") |
| const ( | ||
| AttrController = "usernaut_controller" | ||
| AttrBackend = "usernaut_backend" | ||
| AttrBackendType = "usernaut_backend_type" | ||
| AttrStatus = "usernaut_status" | ||
| AttrOperation = "usernaut_operation" | ||
| AttrError = "usernaut_error" | ||
| ) |
There was a problem hiding this comment.
The attribute keys defined here (e.g., usernaut_controller) deviate from the project's style guide (lines 1431-1445, 1464-1470) and OpenTelemetry semantic conventions. The guide recommends un-prefixed, dot-separated keys like backend.name and status. Using a custom usernaut_ prefix can make metrics less standard and harder to query. Please align with the recommended conventions.
| const ( | |
| AttrController = "usernaut_controller" | |
| AttrBackend = "usernaut_backend" | |
| AttrBackendType = "usernaut_backend_type" | |
| AttrStatus = "usernaut_status" | |
| AttrOperation = "usernaut_operation" | |
| AttrError = "usernaut_error" | |
| ) | |
| const ( | |
| AttrController = "controller.name" | |
| AttrBackend = "backend.name" | |
| AttrBackendType = "backend.type" | |
| AttrStatus = "status" | |
| AttrOperation = "operation" | |
| AttrError = "error.type" | |
| ) |
| func BuildMetricName(baseName, suffix string) string { | ||
| prefixedName := "usernaut_" + baseName | ||
| if suffix == "" { | ||
| return prefixedName | ||
| } | ||
| return prefixedName + suffix | ||
| } |
There was a problem hiding this comment.
The BuildMetricName function constructs metric names using underscores (e.g., usernaut_reconciliation_count). The project's style guide (lines 1411-1418) specifies using dot-separated names to align with OpenTelemetry conventions (e.g., usernaut.reconciliation.count). Using dots improves readability and compatibility with observability platforms. Please update the metric naming to use dots as separators.
| reconciliationMetricsOnce sync.Once | ||
| ) | ||
|
|
||
| type ReconciliationMetrics struct { |
There was a problem hiding this comment.
this file reconciliation.go belongs to the internal/controller package.
packages in go do not "contain" they provide.
this file is providing packages specific to reconciliation. it should be close to reconciler.
| if reconciliationMetrics != nil { | ||
| reconciliationMetrics.RecordReconciliationStart(ctx, controllerName) | ||
| defer func() { | ||
| if err != nil { |
There was a problem hiding this comment.
this error check is tricky.. if i understand this code correctly.. the error value here will be the result from the closure of reconcile func. that way the error will depend on https://github.com/redhat-data-and-ai/usernaut/pull/172/files#diff-4eee68fb1dbeeedac0374c44809b90f9d82a279e98bd5bb84f33e2a6c8ce6985R140 (this err).
instead of this adding a handleError wrapper for all errors in Reconcile func is better.
Changes
📝 Description
What changed?
Why is this change needed?
Dependencies
🧪 Testing
Test Coverage
Performance Impact
🚀 Deployment
Deploy Steps
Prerequisites
Post-Deployment Monitoring
Rollback Plan
Details:
⚙️ Configuration Changes
✅ Developer Checklist