fix(otel): sanitize OTLP endpoint URL resolution#13791
Conversation
|
@greptileai i patched the regex as you requested and tests passing, PR is ready to merge so can you update the score. |
0xRaini
left a comment
There was a problem hiding this comment.
Nice fix! The old endpoint.includes("/v1/") check was too broad — it would incorrectly skip appending the signal path for URLs like https://example.com/opik/api/v1/private/otel where /v1/ is part of the provider's API path, not the OTLP signal path.
What I liked:
- The regex
/\/v1\/(?:traces|metrics|logs)$/is precise — only matches when the path ends with a known OTLP signal segment - Stripping query/fragment before matching is important for correctness
- Excellent test coverage with three distinct scenarios (non-signal /v1, already-qualified, with query params)
suggestion: The split(/[?#]/, 1)[0] ?? endpoint fallback is fine, but split with limit 1 always returns at least one element, so the ?? endpoint is technically unreachable. Not a problem, just a nit — it's good defensive code.
question: Should the regex also handle uppercase signal names (e.g., /v1/Traces)? I've seen some OTLP collectors that aren't strict about case. Probably fine as-is since the official spec uses lowercase, but worth considering.
LGTM! 🎯
@0xRaini added the case-insenstive check, good catch! |
bfc1ccb to
f92900f
Compare
* fix(otel): sanitize OTLP endpoint signal URL resolution * fix(otel): preserve signal URLs with query params * fix(otel): accept case-insensitive signal paths
* fix(otel): sanitize OTLP endpoint signal URL resolution * fix(otel): preserve signal URLs with query params * fix(otel): accept case-insensitive signal paths
* fix(otel): sanitize OTLP endpoint signal URL resolution * fix(otel): preserve signal URLs with query params * fix(otel): accept case-insensitive signal paths
* fix(otel): sanitize OTLP endpoint signal URL resolution * fix(otel): preserve signal URLs with query params * fix(otel): accept case-insensitive signal paths
* fix(otel): sanitize OTLP endpoint signal URL resolution * fix(otel): preserve signal URLs with query params * fix(otel): accept case-insensitive signal paths
Summary
Sanitizes OTLP endpoint resolution so non-signal endpoints that contain internal
/v1/segments still append the correct signal path. Keeps already signal-qualified endpoints unchanged. lobster-biscuitMotivation
This prevents silent cloud export misrouting for valid backends like Opik that use
/api/v1/...in their base URL. Closes #13783Changes
/v1/detection with strict signal-path matching (/v1/traces|/v1/metrics|/v1/logs) before appending./v1/base endpoints and already signal-qualified endpoints.Related
Greptile Overview
Greptile Summary
Fixes OTLP endpoint URL resolution to prevent misrouting for backends that contain internal
/v1/segments (like Opik's/api/v1/...). The change replaces simple substring matching with strict signal-path pattern matching (/v1/traces,/v1/metrics,/v1/logs) that strips query parameters and fragments before testing, ensuring already signal-qualified endpoints remain unchanged while non-signal endpoints get the correct signal path appended.Key improvements:
/v1/substring detection/v1/base endpoints and signal-qualified endpoints with query parametersThe implementation correctly addresses the regression mentioned in the previous thread by extracting the pathname portion for testing while preserving the full URL for the return value.
Confidence Score: 5/5
endpoint.split(/[?#]/, 1)[0]approach. The test suite includes three relevant test cases covering the key scenarios (non-signal/v1/segments, already signal-qualified endpoints, and signal-qualified endpoints with query params). The logic is straightforward and the regex pattern is precise.