feat(spans): Reintroduce OTLP endpoint #4223
Conversation
relay-server/src/endpoints/mod.rs
Outdated
| @@ -74,6 +75,7 @@ pub fn routes(config: &Config) -> Router<ServiceState>{ | |||
| .route("/api/:project_id/minidump/", minidump::route(config)) | |||
| .route("/api/:project_id/events/:event_id/attachments/", post(attachments::handle)) | |||
| .route("/api/:project_id/unreal/:sentry_key/", unreal::route(config)) | |||
| .route("/api/:project_id/traces-data/", traces_data::route(config)) | |||
There was a problem hiding this comment.
Rename to /traces/ to be consistent with https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_traces_protocol?
There was a problem hiding this comment.
Why is it currently "traces-data"?
There was a problem hiding this comment.
In the previous impl it was /spans/ i wanted to name it after the data type it accepts (TracesData) to be consistent with /minidump/, /csp-report/, etc. But reading the link above a bit more, we should maybe even name it /otel/v1/traces/ to be future proof.
For OTLP/HTTP, exporters in the SDK construct signal-specific URLs when this environment variable is set. This means that if you’re sending traces, metrics, and logs, the following URLs are constructed from the example above:
Traces: "http://my-api-endpoint/v1/traces"
Metrics: "http://my-api-endpoint/v1/metrics"
Logs: "http://my-api-endpoint/v1/logs"
https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_endpoint
relay-server/src/endpoints/mod.rs
Outdated
| @@ -74,6 +75,7 @@ pub fn routes(config: &Config) -> Router<ServiceState>{ | |||
| .route("/api/:project_id/minidump/", minidump::route(config)) | |||
| .route("/api/:project_id/events/:event_id/attachments/", post(attachments::handle)) | |||
| .route("/api/:project_id/unreal/:sentry_key/", unreal::route(config)) | |||
| .route("/api/:project_id/traces-data/", traces_data::route(config)) | |||
There was a problem hiding this comment.
Why is it currently "traces-data"?
| @@ -16,16 +22,73 @@ pub use processing::*; | |||
|
|
|||
| pub fn filter(state: &mut ProcessEnvelopeState<SpanGroup>) { | |||
| let disabled = state.should_filter(Feature::StandaloneSpanIngestion); | |||
| let otel_disabled = state.should_filter(Feature::OtelEndpoint); | |||
There was a problem hiding this comment.
We should also check this in the fast path (aka check_envelope).
There was a problem hiding this comment.
We do, via
relay/relay-server/src/services/projects/project/info.rs
Lines 127 to 134 in 63a88f9
Not sure if that check should use should_filter instead of has_feature.
| let Ok(payload) = serde_json::to_vec(&span) else { | ||
| track_invalid(managed_envelope, DiscardReason::Internal); | ||
| continue; | ||
| }; | ||
| let mut item = Item::new(ItemType::OtelSpan); | ||
| item.set_payload(ContentType::Json, payload); | ||
| managed_envelope.envelope_mut().add_item(item); |
There was a problem hiding this comment.
This is really unfortunate, but I can't think of anything better either ...
This brings up the question, is the OtelSpan item always Json, or also allowed to be protobuf?
There was a problem hiding this comment.
You mean the fact that we parse OTLP, serialize it as JSON, and then parse and convert it into our own span format again?
It's definitely suboptimal, but I prefer this over having different code paths for both.
This brings up the question, is the OtelSpan item always Json, or also allowed to be protobuf?
Currently it has to be JSON:
relay/relay-server/src/services/processor/span/processing.rs
Lines 77 to 78 in 63a88f9
| Some(&ContentType::Json) => { | ||
| serde_json::from_slice(&item.payload()).map_err(|_| DiscardReason::InvalidJson) | ||
| } | ||
| Some(&ContentType::Protobuf) => { | ||
| TracesData::decode(item.payload()).map_err(|_| DiscardReason::InvalidProtobuf) | ||
| } |
There was a problem hiding this comment.
I think we should log the errors here in debug, makes debugging issues much much easier.
|
Converting back to draft, unclear if needed. |
Restore the OTLP endpoint that was removed in #3973.
This PR moves the parsing of the trace data to the processor to ensure fast response times.
I also renamed the endpoint from
/spans/to/otlp/v1/traces/to be consistent with https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_endpoint.fixes: https://github.com/getsentry/team-ingest/issues/569