RHINENG-28312: Implement OpenTelemetry - #2311
Open
MichaelMraka wants to merge 10 commits into
Open
Conversation
…ntBased sampler Disabled by default. When enabled, uses OTLP/HTTP, BatchSpanProcessor, service.version from IMAGE_TAG, and rh.service span attributes.
…ume and item links Inventory consume uses extracted traceparent as parent. Bulk produce/item spans use span links so many-to-one batches do not steal a single parent.
Message handlers receive the extracted consumer context. Writes inject W3C traceparent into kafka-go headers so downstream services continue the bulk job trace.
Listener flush starts a new producer span with links to each buffered host span and injects that producer context into Kafka headers.
…st traces The Kafka consumer span is the bulk job parent. Each system evaluation is a child with a span link to the Listener/HBI SpanContext from the payload traceparents array.
…correlation Inbound otelhttp skips probes and metrics. otelsql and otelhttp transport create child spans when request context is passed through.
…o otelsql can wrap connections
Tracing stays off until OTEL_ENABLED is true per environment. IMAGE_TAG is injected so service.version is populated on the OTel resource.
Reviewer's GuideImplements OpenTelemetry-based observability across the service: initializes a configurable OTEL tracer provider, instruments HTTP, Kafka, SQL, and logging, propagates trace context through platform events and evaluators, and wires deployment env vars for per-service telemetry control. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In deploy/clowdapp.yaml under the account-advisory-backfill job, OTEL_SPAN_LINK_COUNT_LIMIT is defined twice (once inside env[] and once as a stray line); the second definition should be removed to avoid YAML/env confusion.
- In listener.event_buffers.flushEvalEvents, the evaluator events are sent with a traced ProducerContext while payload-tracker messages still use base.Context; consider using a similarly instrumented context for payload-tracker sends so those flows participate in the same trace.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In deploy/clowdapp.yaml under the account-advisory-backfill job, OTEL_SPAN_LINK_COUNT_LIMIT is defined twice (once inside env[] and once as a stray line); the second definition should be removed to avoid YAML/env confusion.
- In listener.event_buffers.flushEvalEvents, the evaluator events are sent with a traced ProducerContext while payload-tracker messages still use base.Context; consider using a similarly instrumented context for payload-tracker sends so those flows participate in the same trace.
## Individual Comments
### Comment 1
<location path="deploy/clowdapp.yaml" line_range="64" />
<code_context>
+ - {name: OTEL_BSP_EXPORT_TIMEOUT, value: '${OTEL_BSP_EXPORT_TIMEOUT}'}
+ - {name: OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT, value: '${OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT}'}
+ - {name: OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT, value: '${OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT}'}
+ - {name: OTEL_SPAN_LINK_COUNT_LIMIT, value: '${OTEL_SPAN_LINK_COUNT_LIMIT}'}
resources:
limits: {cpu: '${CPU_LIMIT_ADMIN}', memory: '${MEM_LIMIT_ADMIN}'}
</code_context>
<issue_to_address>
**issue (bug_risk):** Duplicate OTEL_SPAN_LINK_COUNT_LIMIT entry with mismatched indentation is likely to break the YAML structure.
In `account-advisory-backfill`, `OTEL_SPAN_LINK_COUNT_LIMIT` is defined twice: once correctly in the container `env` list and again at the same level as `containers`, which is not valid in the ClowdApp spec. Please remove the mis-indented duplicate and keep only the `env` entry under the container to prevent deploy/parsing issues.
</issue_to_address>
### Comment 2
<location path="base/mqueue/mqueue_test.go" line_range="64-68" />
<code_context>
+ assert.NoError(t, MakeRetryingHandler(handler)(context.Background(), msg))
+}
+
+func TestWriteMessagesInjectsTraceparent(t *testing.T) {
+ // WriteMessages requires a Kafka broker; inject coverage lives in
+ // telemetry.Inject / TestProducerContextLinksOriginalsAndInjectsOwnTraceparent.
+ t.Skip("no broker-free WriteMessages path; covered by telemetry.Inject tests")
}
</code_context>
<issue_to_address>
**suggestion (testing):** Re-evaluate the skipped test for WriteMessages traceparent injection to avoid confusion
This skipped test serves only as a note that `WriteMessages` traceparent behavior is covered via `telemetry.Inject` tests, but appearing as a skipped test can confuse future readers and tooling by implying missing coverage. If feasible, either:
- Implement a real test that verifies header injection without a broker (e.g., using a `kafkaGoWriterImpl` with a dummy `Writer` and checking the headers), or
- Replace the function with a code comment explaining why coverage lives elsewhere.
This will keep the test suite clearer and avoid carrying a permanently skipped, redundant test.
```suggestion
/*
WriteMessages traceparent injection behavior is covered indirectly via telemetry.Inject
tests (e.g., TestProducerContextLinksOriginalsAndInjectsOwnTraceparent in the telemetry
package). The mqueue layer delegates header injection to telemetry.Inject, so adding a
broker-free test here would only duplicate that coverage.
If WriteMessages gains a broker-independent path or local header-injection logic in the
future, consider adding a focused test in this file that exercises those code paths
directly instead of relying on telemetry.Inject.
*/
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+64
to
68
| func TestWriteMessagesInjectsTraceparent(t *testing.T) { | ||
| // WriteMessages requires a Kafka broker; inject coverage lives in | ||
| // telemetry.Inject / TestProducerContextLinksOriginalsAndInjectsOwnTraceparent. | ||
| t.Skip("no broker-free WriteMessages path; covered by telemetry.Inject tests") | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Re-evaluate the skipped test for WriteMessages traceparent injection to avoid confusion
This skipped test serves only as a note that WriteMessages traceparent behavior is covered via telemetry.Inject tests, but appearing as a skipped test can confuse future readers and tooling by implying missing coverage. If feasible, either:
- Implement a real test that verifies header injection without a broker (e.g., using a
kafkaGoWriterImplwith a dummyWriterand checking the headers), or - Replace the function with a code comment explaining why coverage lives elsewhere.
This will keep the test suite clearer and avoid carrying a permanently skipped, redundant test.
Suggested change
| func TestWriteMessagesInjectsTraceparent(t *testing.T) { | |
| // WriteMessages requires a Kafka broker; inject coverage lives in | |
| // telemetry.Inject / TestProducerContextLinksOriginalsAndInjectsOwnTraceparent. | |
| t.Skip("no broker-free WriteMessages path; covered by telemetry.Inject tests") | |
| } | |
| /* | |
| WriteMessages traceparent injection behavior is covered indirectly via telemetry.Inject | |
| tests (e.g., TestProducerContextLinksOriginalsAndInjectsOwnTraceparent in the telemetry | |
| package). The mqueue layer delegates header injection to telemetry.Inject, so adding a | |
| broker-free test here would only duplicate that coverage. | |
| If WriteMessages gains a broker-independent path or local header-injection logic in the | |
| future, consider adding a focused test in this file that exercises those code paths | |
| directly instead of relying on telemetry.Inject. | |
| */ |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2311 +/- ##
==========================================
+ Coverage 58.82% 59.09% +0.26%
==========================================
Files 150 155 +5
Lines 9604 9932 +328
==========================================
+ Hits 5650 5869 +219
- Misses 3360 3451 +91
- Partials 594 612 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Enable configurable OpenTelemetry observability across the service and preserve distributed trace context across HTTP, database, Kafka, evaluation, and logging workflows.
New Features:
Enhancements:
Build:
Deployment:
Tests:
Chores: