feat: add protobuf support for otlp traces#276
Open
AdrianLeeElder wants to merge 4 commits intodevelopfrom
Open
feat: add protobuf support for otlp traces#276AdrianLeeElder wants to merge 4 commits intodevelopfrom
AdrianLeeElder wants to merge 4 commits intodevelopfrom
Conversation
📝 WalkthroughWalkthroughAdds protobuf decoding for OTLP logs, traces, and metrics: new dependency for OTLP proto types, a protobuf parser utility, protobuf parsing methods in services, and route handlers dispatching on Content-Type between JSON and protobuf (with gzip decompression preserved). Tests for protobuf paths were added or updated. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RouteHandler
participant Decompression
participant ContentCheck
participant ProtobufService
participant JsonService
participant Utils as OtlpProtobufParser
participant Downstream as IngestQueue
Client->>RouteHandler: POST /v1/{logs,traces,metrics}\nContent-Type: application/x-protobuf (maybe gzip)
RouteHandler->>ContentCheck: inspect Content-Type
alt protobuf
RouteHandler->>Decompression: decompress(payload)
Decompression-->>RouteHandler: bytes
RouteHandler->>ProtobufService: parseFrom(bytes)
ProtobufService->>Utils: extractResourceContext / attributesToMap / bytesToHex
Utils-->>ProtobufService: normalized fields
ProtobufService-->>RouteHandler: List<domain inserts>
else json
RouteHandler->>Decompression: decompress(payload)
Decompression-->>RouteHandler: string
RouteHandler->>JsonService: parseJson(string)
JsonService-->>RouteHandler: List<domain inserts>
else unsupported
RouteHandler-->>Client: 415 Unsupported Media Type
end
RouteHandler->>Downstream: enqueue(entries)
Downstream-->>RouteHandler: ack
RouteHandler-->>Client: 200/202 Accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/src/main/kotlin/com/moneat/otlp/routes/OtlpMetricsRoutes.kt (1)
101-107:⚠️ Potential issue | 🟡 MinorUpdate error message to be format-agnostic.
The error message on line 104 references "malformed JSON" but now applies to both JSON and protobuf payloads.
📝 Proposed fix
if (parsedMetrics == null) { call.respond( HttpStatusCode.BadRequest, - ErrorResponse("Invalid OTLP metrics payload: malformed JSON or missing resourceMetrics") + ErrorResponse("Invalid OTLP metrics payload: malformed request or missing resourceMetrics") ) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/otlp/routes/OtlpMetricsRoutes.kt` around lines 101 - 107, The error message in the parsedMetrics null-check is JSON-specific; update the call.respond ErrorResponse used in the parsedMetrics == null branch (the ErrorResponse construction invoked via call.respond) to a format-agnostic message such as "Invalid OTLP metrics payload: malformed or missing resourceMetrics" so it applies to both JSON and protobuf payloads; leave the parsedMetrics null-check logic unchanged and only change the string passed to ErrorResponse.
🧹 Nitpick comments (3)
backend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt (1)
513-517: Consider extracting sharedkv()helper to reduce duplication.The
kv()helper function is duplicated betweenLogServicePureLogicTest.kt(lines 513-517) andOtlpTraceServiceTest.kt(lines 499-503). Consider extracting it to a shared test utility.Also applies to: 499-503
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt` around lines 513 - 517, Extract the duplicated kv() helper into a shared test utility so both LogServicePureLogicTest and OtlpTraceServiceTest call the same function: create a test helper (e.g., TestKeyValueUtils.kt) that exposes fun kv(key: String, value: String): KeyValue using KeyValue.newBuilder().setKey(key).setValue(AnyValue.newBuilder().setStringValue(value)).build(), replace the two local kv definitions with calls to that helper, and update imports in both test files to reference the new utility.backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt (1)
505-545: Consider adding span kind coverage for protobuf parsing.The JSON parsing tests at lines 129-162 verify all span kinds (0-5), but the protobuf tests only exercise
SPAN_KIND_SERVER. Consider adding a parameterized test or additional cases to ensuremapSpanKindworks correctly for protobuf-originatedkindValue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt` around lines 505 - 545, Add coverage for all protobuf span kinds by extending the test for parseOtlpTracesProtobuf (e.g., parameterize or add cases to the `parses single span from protobuf` test) to iterate over Span.SpanKind values (or their numeric kindValue 0-5) and assert the mapped result from mapSpanKind/parsed span.kind matches expected strings (e.g., "UNSPECIFIED","INTERNAL","SERVER","CLIENT","PRODUCER","CONSUMER"); locate usage of Span.SpanKind and parseOtlpTracesProtobuf in OtlpTraceServiceTest and create one span per kind with identical other fields, then assert s.kind for each case.backend/src/main/kotlin/com/moneat/logs/routes/LogRoutes.kt (1)
692-700: Note: Different error semantics between logs and traces parsers.The logs parser (
parseOtlpProtobuf) returns an empty list on parse failure, while traces (parseOtlpTracesProtobuf) returnsnull. This works correctly here since an empty payload is a valid case, but the inconsistency could cause confusion if someone expects uniform behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/main/kotlin/com/moneat/logs/routes/LogRoutes.kt` around lines 692 - 700, The behavior of the logs parsers is inconsistent with traces: update the log parsing API so parseOtlpProtobuf and parseOtlpJson return null on parse failure (like parseOtlpTracesProtobuf) and only return an empty list for a successfully parsed but empty payload, or alternatively normalize in the caller by treating both null and empty as parse-failure; change the implementations in LogService (parseOtlpProtobuf, parseOtlpJson) to return a nullable list and ensure the caller code that checks parsedEntries handles null vs empty consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/otlp/services/OtlpTraceService.kt`:
- Around line 381-382: escapeJsonString is missing escapes for backspace and
form feed; update the function (escapeJsonString) to also replace '\b' and '\f'
with their JSON escapes (e.g., add .replace("\b", "\\b").replace("\f", "\\f") in
the chain) so all control characters valid in JSON are escaped.
---
Outside diff comments:
In `@backend/src/main/kotlin/com/moneat/otlp/routes/OtlpMetricsRoutes.kt`:
- Around line 101-107: The error message in the parsedMetrics null-check is
JSON-specific; update the call.respond ErrorResponse used in the parsedMetrics
== null branch (the ErrorResponse construction invoked via call.respond) to a
format-agnostic message such as "Invalid OTLP metrics payload: malformed or
missing resourceMetrics" so it applies to both JSON and protobuf payloads; leave
the parsedMetrics null-check logic unchanged and only change the string passed
to ErrorResponse.
---
Nitpick comments:
In `@backend/src/main/kotlin/com/moneat/logs/routes/LogRoutes.kt`:
- Around line 692-700: The behavior of the logs parsers is inconsistent with
traces: update the log parsing API so parseOtlpProtobuf and parseOtlpJson return
null on parse failure (like parseOtlpTracesProtobuf) and only return an empty
list for a successfully parsed but empty payload, or alternatively normalize in
the caller by treating both null and empty as parse-failure; change the
implementations in LogService (parseOtlpProtobuf, parseOtlpJson) to return a
nullable list and ensure the caller code that checks parsedEntries handles null
vs empty consistently.
In `@backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt`:
- Around line 505-545: Add coverage for all protobuf span kinds by extending the
test for parseOtlpTracesProtobuf (e.g., parameterize or add cases to the `parses
single span from protobuf` test) to iterate over Span.SpanKind values (or their
numeric kindValue 0-5) and assert the mapped result from mapSpanKind/parsed
span.kind matches expected strings (e.g.,
"UNSPECIFIED","INTERNAL","SERVER","CLIENT","PRODUCER","CONSUMER"); locate usage
of Span.SpanKind and parseOtlpTracesProtobuf in OtlpTraceServiceTest and create
one span per kind with identical other fields, then assert s.kind for each case.
In `@backend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt`:
- Around line 513-517: Extract the duplicated kv() helper into a shared test
utility so both LogServicePureLogicTest and OtlpTraceServiceTest call the same
function: create a test helper (e.g., TestKeyValueUtils.kt) that exposes fun
kv(key: String, value: String): KeyValue using
KeyValue.newBuilder().setKey(key).setValue(AnyValue.newBuilder().setStringValue(value)).build(),
replace the two local kv definitions with calls to that helper, and update
imports in both test files to reference the new utility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f16f7baf-a2b7-4160-a82c-398278263199
📒 Files selected for processing (14)
backend/build.gradle.ktsbackend/gradle/libs.versions.tomlbackend/src/main/kotlin/com/moneat/logs/routes/LogRoutes.ktbackend/src/main/kotlin/com/moneat/logs/services/LogService.ktbackend/src/main/kotlin/com/moneat/otlp/OtlpProtobufParser.ktbackend/src/main/kotlin/com/moneat/otlp/routes/OtlpMetricsRoutes.ktbackend/src/main/kotlin/com/moneat/otlp/routes/OtlpTraceRoutes.ktbackend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.ktbackend/src/main/kotlin/com/moneat/otlp/services/OtlpTraceService.ktbackend/src/test/kotlin/com/moneat/otlp/OtlpProtobufParserTest.ktbackend/src/test/kotlin/com/moneat/otlp/services/OtlpMetricsServiceTest.ktbackend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.ktbackend/src/test/kotlin/com/moneat/routes/LogRoutesTest.ktbackend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt
backend/src/main/kotlin/com/moneat/otlp/services/OtlpTraceService.kt
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt (1)
461-593: Consider adding test coverage for protobuf events and links.The
ProtobufParsingtests thoroughly cover span parsing, error handling, and edge cases. However, the newprotoEventsToJsonandprotoLinksToJsonhelper functions are not exercised by any tests. Consider adding a test that includes spans with events and links to verify the JSON serialization logic.💡 Example test for events and links
`@Test` fun `parses span with events and links`() { val event = Span.Event.newBuilder() .setTimeUnixNano(1700000000000000000L) .setName("exception") .addAttributes(kv("exception.type", "RuntimeException")) .build() val link = Span.Link.newBuilder() .setTraceId(traceIdBytes()) .setSpanId(spanIdBytes()) .addAttributes(kv("link.type", "parent")) .build() val bytes = buildRequest( spanBuilder = Span.newBuilder() .setTraceId(traceIdBytes()) .setSpanId(spanIdBytes()) .setName("span-with-events-links") .addEvents(event) .addLinks(link) ) val spans = service.parseOtlpTracesProtobuf(bytes)!! assertEquals(1, spans.size) assertTrue(spans[0].events.contains("exception")) assertTrue(spans[0].events.contains("exception.type")) assertTrue(spans[0].links.contains("0af7651916cd43dd8448eb211c80319c")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt` around lines 461 - 593, Tests in ProtobufParsing don't cover the new protoEventsToJson and protoLinksToJson helpers; add a test in OtlpTraceServiceTest.ProtobufParsing that builds a span containing events and links (use Span.newBuilder().addEvents(...) and .addLinks(...)), serialize it with the existing buildRequest helper, call service.parseOtlpTracesProtobuf(bytes) and assert that the resulting span's JSON/fields include the event name and attributes and the linked trace/span IDs (refer to protoEventsToJson, protoLinksToJson and parseOtlpTracesProtobuf to validate expected keys/values).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.kt`:
- Around line 135-139: In OtlpMetricsService (around
ExportMetricsServiceRequest.parseFrom(bytes)) replace the broad catch of
Exception with catching com.google.protobuf.InvalidProtocolBufferException
specifically, and stop passing the exception to logger.warn (which emits a stack
trace); instead log a bounded message like logger.warn { "Invalid OTLP protobuf
metrics payload: ${e.message?.take(500)}" } and add the proper import for
InvalidProtocolBufferException; apply the same pattern for similar parseFrom
usages (e.g., in OtlpTraceService) to avoid full stack-trace logging.
---
Nitpick comments:
In `@backend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.kt`:
- Around line 461-593: Tests in ProtobufParsing don't cover the new
protoEventsToJson and protoLinksToJson helpers; add a test in
OtlpTraceServiceTest.ProtobufParsing that builds a span containing events and
links (use Span.newBuilder().addEvents(...) and .addLinks(...)), serialize it
with the existing buildRequest helper, call
service.parseOtlpTracesProtobuf(bytes) and assert that the resulting span's
JSON/fields include the event name and attributes and the linked trace/span IDs
(refer to protoEventsToJson, protoLinksToJson and parseOtlpTracesProtobuf to
validate expected keys/values).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bfd7ee66-65fa-4ddd-8db2-222265cf5b8a
📒 Files selected for processing (6)
backend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.ktbackend/src/main/kotlin/com/moneat/otlp/services/OtlpTraceService.ktbackend/src/test/kotlin/com/moneat/otlp/OtlpProtobufParserTest.ktbackend/src/test/kotlin/com/moneat/otlp/services/OtlpMetricsServiceTest.ktbackend/src/test/kotlin/com/moneat/otlp/services/OtlpTraceServiceTest.ktbackend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt
✅ Files skipped from review due to trivial changes (1)
- backend/src/test/kotlin/com/moneat/otlp/OtlpProtobufParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/src/test/kotlin/com/moneat/otlp/services/OtlpMetricsServiceTest.kt
- backend/src/test/kotlin/com/moneat/services/LogServicePureLogicTest.kt
Comment on lines
+135
to
+139
| try { | ||
| ExportMetricsServiceRequest.parseFrom(bytes) | ||
| } catch (e: Exception) { | ||
| logger.warn(e) { "Invalid OTLP protobuf metrics payload" } | ||
| return null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify broad catches and throwable-based warn logging around protobuf decoding.
rg -nP --type=kt 'parseFrom\(|catch\s*\(e:\s*Exception\)|logger\.warn\s*\(e\)'Repository: moneat-io/moneat
Length of output: 86
🏁 Script executed:
#!/bin/bash
# Read the specific file and lines mentioned in the review
cat -n backend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.kt | sed -n '130,145p'Repository: moneat-io/moneat
Length of output: 803
🏁 Script executed:
#!/bin/bash
# Search for protobuf parsing patterns and exception handling in Kotlin files
rg -n 'parseFrom|catch.*Exception|logger\.warn\s*\(' backend/src/main/kotlin/com/moneat/otlp/services/ -A 2 -B 1Repository: moneat-io/moneat
Length of output: 8467
🏁 Script executed:
#!/bin/bash
# Check for InvalidProtocolBufferException imports in the file
grep -n 'import.*protobuf\|InvalidProtocolBufferException' backend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.ktRepository: moneat-io/moneat
Length of output: 42
Narrow protobuf exception handling to avoid full stack-trace logging.
Catching Exception here masks non-decode failures, and logger.warn(e) emits the full exception with stack trace. Catch InvalidProtocolBufferException explicitly and log only a bounded message using .take(500) per the coding guideline.
🔧 Proposed fix
+import com.google.protobuf.InvalidProtocolBufferException
+
try {
ExportMetricsServiceRequest.parseFrom(bytes)
- } catch (e: Exception) {
- logger.warn(e) { "Invalid OTLP protobuf metrics payload" }
+ } catch (e: InvalidProtocolBufferException) {
+ logger.warn { "Invalid OTLP protobuf metrics payload: ${(e.message ?: "").take(500)}" }
return null
}Note: The same pattern appears in OtlpTraceService.kt and other parsing methods (JSON payloads).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| ExportMetricsServiceRequest.parseFrom(bytes) | |
| } catch (e: Exception) { | |
| logger.warn(e) { "Invalid OTLP protobuf metrics payload" } | |
| return null | |
| try { | |
| ExportMetricsServiceRequest.parseFrom(bytes) | |
| } catch (e: InvalidProtocolBufferException) { | |
| logger.warn { "Invalid OTLP protobuf metrics payload: ${(e.message ?: "").take(500)}" } | |
| return null | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/main/kotlin/com/moneat/otlp/services/OtlpMetricsService.kt`
around lines 135 - 139, In OtlpMetricsService (around
ExportMetricsServiceRequest.parseFrom(bytes)) replace the broad catch of
Exception with catching com.google.protobuf.InvalidProtocolBufferException
specifically, and stop passing the exception to logger.warn (which emits a stack
trace); instead log a bounded message like logger.warn { "Invalid OTLP protobuf
metrics payload: ${e.message?.take(500)}" } and add the proper import for
InvalidProtocolBufferException; apply the same pattern for similar parseFrom
usages (e.g., in OtlpTraceService) to avoid full stack-trace logging.
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
1 participant
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.
Adds protobuf support for OTLP traces, in addition to JSON support.
Fixes #264
Summary by CodeRabbit
New Features
Chores
Tests