fix(observability-tracing-openobserve): return empty result when stream not yet created#114
fix(observability-tracing-openobserve): return empty result when stream not yet created#114mvanhorn wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ErrStreamNotFound sentinel and detection helper, makes executeSearchQuery return it when OpenObserve signals a missing stream, and updates GetTraces/GetSpans/GetSpanDetail to treat that case as "no data" (or "span not found"); tests and a Helm chart version bump included. ChangesStream-not-found error handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@observability-tracing-openobserve/internal/openobserve/client_test.go`:
- Around line 971-1046: Add a unit test for GetSpanDetail that verifies
stream-not-found is translated into a "span not found" error: spin up an
httptest server that returns HTTP 400 with body {"code":20002,"message":"Search
stream not found: default"}, create the client with newTestClient(server.URL),
call client.GetSpanDetail(context.Background(),
TracesQueryParams{TraceID:"trace-1", SpanID:"span-1"}), assert that err is
non-nil and that err.Error() contains both "span" and "not found" (use
TestGetSpanDetail_StreamNotFound as the test name to match existing style).
- Around line 985-1003: Add tests that exercise the count-query-only
stream-not-found path: create TestGetTraces_StreamNotFound_CountQueryOnly (and a
symmetric TestGetSpans_StreamNotFound_CountQueryOnly) that spins up an
httptest.Server which returns a successful body for the main query endpoint
(containing one or more traces/spans) but returns HTTP 400 with JSON
{"code":20002,...} for the count endpoint; call client.GetTraces /
client.GetSpans (using newTestClient and TracesQueryParams/SpansQueryParams),
assert err == nil, result is non-nil, len(result.Traces or result.Spans) matches
the returned items and result.Total == len(result.Traces or result.Spans) to
verify the sentinel on the count query sets Total to the number of returned
items.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5cc3f913-f93f-4be1-9021-2b8c11a65af5
📒 Files selected for processing (2)
observability-tracing-openobserve/internal/openobserve/client.goobservability-tracing-openobserve/internal/openobserve/client_test.go
…am not yet created OpenObserve returns HTTP 400 with code 20002 when a search target stream has not yet been created (the stream is lazy - created on first write). The adapter currently treats this as a hard failure, which surfaces as HTTP 500 from the observer API. "No data yet" is not a server error; the API should return an empty result set. Adds a sentinel ErrStreamNotFound and a tolerant isStreamNotFound helper that parses the OpenObserve error envelope. executeSearchQuery returns the sentinel when the body says code == 20002; GetTraces, GetSpans, and GetSpanDetail each translate the sentinel into the appropriate zero-value result. Companion to openchoreo/openchoreo#3439 - moves the OpenObserve-specific error-code handling out of the observer interface (which should stay adapter-agnostic) and into the adapter that actually knows the codes, per @akila-i's review feedback. Refs openchoreo/openchoreo#3435 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
6edec74 to
8061aa1
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…m-not-found test coverage Addresses CI + review feedback on openchoreo#114: - helm/Chart.yaml: bump version + appVersion from 0.2.2 to 0.2.3 so check-version-bump accepts the module changes. - client_test.go: add three tests coderabbit flagged as missing on the review. - TestGetSpanDetail_StreamNotFound verifies the sentinel maps to a "span not found" error. - TestGetTraces_StreamNotFound_CountQueryOnly and the spans variant verify the documented partial-failure path: main query succeeds, count query returns code 20002, GetTraces / GetSpans return the returned hits with Total = len(results) and err == nil. Signed-off-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Purpose
Moves the OpenObserve empty-stream handling from the observer interface (
openchoreo/openchoreo) into this adapter, per @akila-i's review feedback on openchoreo/openchoreo#3439. The observer interface stays adapter-agnostic; OpenObserve-specific codes (20002 = "stream not found") live in the adapter that actually knows them.Refs:
Bug
When a component has never emitted spans, OpenObserve responds to a search with HTTP 400 and body
{"code":20002,"message":"Search stream not found: default"}because the search stream is created lazily on first write. The adapter'sexecuteSearchQuerycurrently treats any non-200 as a hard failure, so "no data yet" surfaces as HTTP 500 to the API client through the observer interface.Approach
Single file edit on the adapter side, plus tests. All changes are confined to
observability-tracing-openobserve/internal/openobserve/.openObserveStreamNotFoundCode = 20002and a smallisStreamNotFound([]byte) boolhelper that parses the OpenObserve error envelope. Tolerant: empty body, malformed JSON, or JSON withoutcodeall return false, so existing error paths are unchanged for everything except this one specific OpenObserve response.ErrStreamNotFound(Go convention for "expected non-error state", likesql.ErrNoRows).executeSearchQueryreturns the sentinel when the body indicates code 20002.GetTraces,GetSpans,GetSpanDetail) translates the sentinel into the appropriate zero-value result andnilerror. ForGetTraces/GetSpans, if only the count query returns the sentinel (rare), the search results are returned withTotal = len(results)rather than failing.Tests
Four new tests in
client_test.go:TestExecuteSearchQuery_StreamNotFound- asserts the sentinel is returned for a 400+code:20002 response.TestGetTraces_StreamNotFound- asserts the public API returns an empty*TracesResultandnilerror.TestGetSpans_StreamNotFound- symmetric for spans.TestIsStreamNotFound- table-driven test for the parser (matching code, other code, missing code, empty body, malformed JSON).All pass; existing tests unaffected;
go vetclean.What this doesn't change
openchoreo/openchoreo. The companion PR there (#3439) can now be closed in favor of this one - the OpenObserve-specific behavior lives where it should.observability-logs-openobserveandobservability-metrics-*if you'd like.This contribution was developed with AI assistance (Claude Code).
Summary by CodeRabbit
Bug Fixes
Tests
Chores