coderabbitai[bot]
commented
1 week ago Walkthrough
The changes in this pull request involve modifications to the observability_test.go, otel.go, helpers.go, and obsy.go files. In observability_test.go, several constants and variables are updated for improved configurability, including the introduction of a new scrapeInterval constant and a more dynamic command string. In otel.go, a new LoggingExporter struct is introduced, along with methods for handling logging exporters. Additionally, helpers.go adds a method for setting the logging exporter, and obsy.go introduces a new configuration field for the logging exporter log level.
Changes
| File Path | Change Summary |
|---|---|
| e2e/basic/observability_test.go | - Updated TestObservabilityCollector with dynamic prometheusArgs and added scrapeInterval. - Modified targetStartCommand for dynamic otlpPort. - Changed scrape_interval to use scrapeInterval. - Updated target instance name format. - Reduced wait time from 60 seconds to 20 seconds. - Added TestObservabilityCollectorWithLogging for logging verification. |
| pkg/sidecars/observability/otel.go | - Introduced LoggingExporter struct with LogLevel field. - Added createLoggingExporter method. - Updated methods to append logging exporter to metrics and traces pipelines. - Added logic to reset metrics and traces if no exporters are present. |
| pkg/sidecars/observability/helpers.go | - Added SetLoggingExporter method to set logging exporter with validation and default handling. |
| pkg/sidecars/observability/obsy.go | - Added loggingExporterLogLevel field to ObsyConfig struct for logging configuration. |
| e2e/system/build_image_test.go | - Updated expectedData in TestBuildWithBuildArgs from "Hello, build arg!" to "Hello, World!". |
Sequence Diagram(s)
sequenceDiagram
participant Test as TestObservabilityCollector
participant Prometheus as Prometheus Config
participant Service as Service
participant Telemetry as Telemetry Struct
participant Logging as LoggingExporter
Test->>Prometheus: Set prometheusArgs with dynamic config
Prometheus-->>Test: Return updated config
Test->>Service: Initialize Service with Telemetry
Service->>Telemetry: Set Address and Level
Telemetry-->>Service: Return initialized Telemetry
Service->>Test: Return Service with valid metrics and traces
Test->>Logging: Set LoggingExporter with log level
Logging-->>Test: Confirm LoggingExporter setup🐇 "In the code where changes abound,
New structures and constants are found.
With logging now in the mix,
Our observability gets its fix!
From waits that were long, now just a short hop,
Our tests are now ready, let’s make them pop!" 🐇
Possibly related PRs
- #569: The changes in this PR involve enhancements to the handling of build arguments when building Docker images, which may relate to the dynamic configuration aspects introduced in the main PR's observability tests, particularly in how configurations are set and utilized.
- #577: This PR adds end-to-end tests for sidecars, which may connect to the main PR's focus on observability and logging within the context of sidecars, enhancing the overall testing framework and functionality.
Suggested reviewers
- smuu
- MSevey
📜 Recent review details
**Configuration used: CodeRabbit UI** **Review profile: CHILL**📥 Commits
Reviewing files that changed from the base of the PR and between 495a4d472e9473f85c173c20450e32eeff6449dd and 6336782422020d01e4ead10ac95bd2555c458b27.📒 Files selected for processing (1)
* `e2e/system/build_image_test.go` (1 hunks)🔇 Additional comments (1)
e2e/system/build_image_test.go (1)
`103-103`: _:warning: Potential issue_ **Test might not effectively validate build arguments.** The change to `"Hello, World!"` weakens the test's ability to verify that build arguments are working correctly. A more distinctive value would better prove that the build argument is actually being used in the build process. Let's verify the Dockerfile contents to understand how the MESSAGE build arg is used: Suggestion: Consider changing back to a more distinctive message or using a unique format like: ```diff - expectedData = "Hello, World!" + expectedData = "BuildArg_Test_Message_12345" ``` This would make it clear that the value comes from the build argument rather than being hardcoded in the Dockerfile.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?
There was an issue with the obsy sidecar and the test which was blocking the celestia-node integration. This PR proposes a solution for it.
Thank you @smuu for helping in finding the bug.
Summary by CodeRabbit
New Features
Bug Fixes