fix: obsy prometheus exporter #579
Conversation
WalkthroughThe changes introduce a new Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
pkg/sidecars/observability/otel.go (2)
Line range hint
193-204: LGTM! Consider adding documentation for the Telemetry struct.The new Telemetry and MetricsTelemetry structs are well-defined. The Level field's comment is helpful, but consider adding godoc comments for the structs themselves to explain their purpose and usage.
Add documentation like:
// Telemetry defines the configuration for OpenTelemetry's own telemetry data type Telemetry struct { Metrics MetricsTelemetry `yaml:"metrics,omitempty"` } // MetricsTelemetry configures how OpenTelemetry's own metrics are exposed type MetricsTelemetry struct { Address string `yaml:"address,omitempty"` // The address where telemetry metrics are exposed Level string `yaml:"level,omitempty"` // Options are basic, normal, detailed }
Line range hint
437-452: LGTM! Consider making telemetry configuration more flexible.The telemetry configuration is properly initialized with default values. However, consider making the address and level configurable through obsyConfig instead of using hardcoded constants.
Consider modifying the code to allow configuration:
telemetry := Telemetry{ Metrics: MetricsTelemetry{ - Address: internalTelemetryEndpoint, - Level: basicTelemetryLevel, + Address: o.obsyConfig.telemetryAddress, + Level: o.obsyConfig.telemetryLevel, }, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
pkg/sidecars/observability/otel.go(1 hunks)
🔇 Additional comments (2)
pkg/sidecars/observability/otel.go (2)
346-349: LGTM! Improved Prometheus exporter configuration.
The change from checking port to checking endpoint is more flexible and consistent with how other exporters are configured. This fix properly addresses the PR objective of enabling Prometheus as an exporter.
346-349: Verify backward compatibility for existing users.
The change from port-based to endpoint-based configuration for Prometheus exporter might affect existing users. Please ensure:
- Documentation is updated to reflect this change
- Migration guide is provided if needed
- Default values are handled appropriately
Also applies to: 437-452
✅ Verification successful
No breaking changes detected in Prometheus configuration
The codebase shows a clean transition:
- The old
prometheusEndpointPortis still maintained alongside the newprometheusExporterEndpoint - Both configurations are handled independently with proper null checks
- Test files demonstrate proper default values and configuration options
- The changes are additive rather than replacing existing functionality
No documentation updates are required as this appears to be a new feature addition rather than a breaking change.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing usages of prometheusEndpointPort to identify potential breaking changes
rg "prometheusEndpointPort" --type go
# Search for documentation files that might need updates
fd -e md -e rst -e txt | xargs rg -l "prometheus.*port"
Length of output: 756
Script:
#!/bin/bash
# Check for the new prometheusExporterEndpoint configuration
rg "prometheusExporterEndpoint" --type go -A 2
# Check for any configuration or setup examples in documentation
fd . -e md -e yaml -e yml | xargs rg -l "prometheus"
# Look for test files to understand default behavior
rg "createPrometheusExporter|NewObservability" --type go -A 5
Length of output: 2389
There was a bug that I was trying to find and the culprit is in the obsy sidecar. It prevented to set Prometheus as an exporter in the Otel collector. This PR proposes a fix for that.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation