feat(data-pipeline): add agentless export#2081
feat(data-pipeline): add agentless export#2081gh-worker-dd-mergequeue-cf854d[bot] merged 18 commits into
Conversation
# Motivation Send traces to the agentless endpoint to replace the JS and python exporters # What changes * Additional JSON agentless encoder * Additional configuration (agentless enablement, endpoint, API key) * Agentless traces need top level computation, but not dropping spans
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf49210179
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2081 +/- ##
==========================================
+ Coverage 73.58% 73.73% +0.14%
==========================================
Files 470 474 +4
Lines 78449 79241 +792
==========================================
+ Hits 57729 58425 +696
- Misses 20720 20816 +96
🚀 New features to boost your workflow:
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 1d15342 | Docs | Datadog PR Page | Give us feedback! |
| self.agentless_endpoint.as_ref(), | ||
| ) { | ||
| (Some(_), Some(_)) => { | ||
| tracing::warn!( |
There was a problem hiding this comment.
Should the builder fail if it has conflicting configuration? SDK configuration is complex. I'm of the opinion that it's better for the complexity to be concentrated in the SDKs and let them handle things like precedence.
There was a problem hiding this comment.
On the same topic, the build should probably error out if set_url and output_format is used and agentless is set at the same time.
| ), | ||
| }; | ||
|
|
||
| let agentless_config = match (agentless_endpoint, agentless_api_key) { |
There was a problem hiding this comment.
agentless config is going to happen after AgentInfoFetcher is spawned here. Won't that fail every 5 minutes when it polls an agent that doesn't exist?
There was a problem hiding this comment.
I think the same applies to telemetry? It's built against the agent url.
There was a problem hiding this comment.
I think all of this also applies to otlp mode for the exporter.
There was a problem hiding this comment.
Made agentless mode incompatible with OLTP and agent.
OTLP is not incompatible with the agent settings though (and maybe shoudln't be? Users could be using the datadog agent still so we still get telemetry, but decide to export traces in otlp format somewhere else)
| for (k, v) in span.meta_struct.iter() { | ||
| let key: &str = k.borrow(); | ||
| let bytes: &[u8] = v.borrow(); | ||
| // Encode as a JSON array of u8 (default serde behavior for &[u8]). |
There was a problem hiding this comment.
Is this the right thing to do? If i'm reading js correctly (a big if) they just send meta_struct values as json, not msgpack encoded bytes.
There was a problem hiding this comment.
fixed, we now transcode the msp bytes to a json object
| } | ||
|
|
||
| /// Sends trace chunks to the Datadog agentless intake (`/v1/input`) as JSON. | ||
| async fn send_agentless_traces_inner<T: TraceData>( |
There was a problem hiding this comment.
Where is max payload size handled? In the SDKs? I assume the endpoint has a max size that we have to respect?
There was a problem hiding this comment.
It has a 5MB size limit I believe. I don't think this limit should be enforced in the trace exporter though.
It should probably be at the trace buffer.
Or maybe we could split incoming list of trace chunks in multiple payloads 🤔
There was a problem hiding this comment.
Ah, that's a good point. The trace buffer should handle flushing below the limit.
Although, looking at the trace buffer in data pipeline we don't enforce limits or split up chunks. We check after a chunk is added, and flush above flush_trigger_bytes. It's theoretically possible that if we receive a significantly large chunk we flush something that exceeds the intake limit. In a similar vain, we only drop chunks after max_buffered_bytes is exceeded, which can also trigger a flush that's beyond intake's limit.
What may make this an issue for agentless is that the max size for the agent is significantly larger than intake's limit. And the agent handles splitting up the payloads for intake's limits. So the trace buffers can usually get away with "trigger a flush once we exceed a limit".
I'm not sure this is a practical problem today that we need to deal with in this PR? I think we can tackle it separately?
|
I have some questions:
This PR feels like a lot of re-inventing the wheel. |
Because it is not the same schema. The agentless endpoint expects something like As to why I do the conversion at encoding, creating an
I mean, I agree that the api key should be passed in the |
|
Makes sense, there is quite some overhead. The v1 protocol is much better at being transformed from what agent expects to what intake expects. I would prefer though if this weren't part of data-pipeline, but SendData as well. (i.e. just moving the code there). |
…e/agentless_export
e358423 to
9b505b4
Compare
This test was asserting that not setting the url on the ffi failed. But this does not match the rust API as in the non-ffi builder which is wrapped, we use the default localhost url of the agent if none is provided
| //! `metrics["_trace_root"]=1` where applicable. | ||
| //! - **Non-finite metrics** (NaN/Inf) are dropped (JSON can't represent them). | ||
| //! | ||
| //! Left todo is span normalization (service/name/resource/type truncation + defaults) |
There was a problem hiding this comment.
Is this a blocker to release ?
There was a problem hiding this comment.
minor: Would you mind changing this to // TODO: ... and adding a Jira ticket to track it?
ekump
left a comment
ekump
left a comment
There was a problem hiding this comment.
A few potential follow-ups, but nothing blocking. LGTM.
| //! - **Mutual exclusion with OTLP**: if both an OTLP and an agentless endpoint are configured on | ||
| //! the builder, OTLP wins and the agentless config is silently dropped with a warning at build | ||
| //! time. |
There was a problem hiding this comment.
| //! - **Mutual exclusion with OTLP**: if both an OTLP and an agentless endpoint are configured on | |
| //! the builder, OTLP wins and the agentless config is silently dropped with a warning at build | |
| //! time. | |
| //! - **Mutually exclusive transport**: agentless cannot be combined with OTLP | |
| //! ([`set_otlp_endpoint`]), a caller-supplied agent URL ([`set_url`]), or log output; | |
| //! configuring any of them together causes `build`/`build_async` to return | |
| //! `BuilderErrorKind::InvalidConfiguration`. |
| } | ||
| } | ||
|
|
||
| #[cfg_attr(miri, ignore)] |
There was a problem hiding this comment.
We still create an InvalidUrl error if you pass an empty string. We should probably continue to test that?
| /// Agentless trace export is mutually exclusive with both OTLP trace export | ||
| /// ([`Self::set_otlp_endpoint`]) and a configured agent URL ([`Self::set_url`]); | ||
| /// combining either with this method causes [`Self::build`]/[`Self::build_async`] | ||
| /// to return [`BuilderErrorKind::InvalidConfiguration`]. |
There was a problem hiding this comment.
| /// to return [`BuilderErrorKind::InvalidConfiguration`]. | |
| /// to return [`BuilderErrorKind::InvalidConfiguration`]. | |
| /// the output format is ignored in agentless mode; payloads are always | |
| /// JSON |
| //! `metrics["_trace_root"]=1` where applicable. | ||
| //! - **Non-finite metrics** (NaN/Inf) are dropped (JSON can't represent them). | ||
| //! | ||
| //! Left todo is span normalization (service/name/resource/type truncation + defaults) |
There was a problem hiding this comment.
minor: Would you mind changing this to // TODO: ... and adding a Jira ticket to track it?
| /// | ||
| /// Returns `None` if serialization fails. The result is truncated to | ||
| /// [`MAX_META_VALUE_LEN`] characters with a trailing `"..."` if it would | ||
| /// otherwise exceed that limit. |
There was a problem hiding this comment.
It's ok to defer to another PR, but if we truncate it's going to result in malformed JSON and get rejected at intake. I think #980 also introduces the same issue.
| let key: &str = k.borrow(); | ||
| let bytes: &[u8] = v.borrow(); | ||
|
|
||
| // abort whole payload on malformed entry |
There was a problem hiding this comment.
Ok to defer to another PR: Should we really be aborting the entire payload if a single span's meta_struct fails to serialize? I think dd-trace-js will drop just the span.
There was a problem hiding this comment.
The issue is that there is no easy way to both:
- Not add overhead by going though an intermediary buffer
- Rollback partial invalid writes
In practice since meta struct bytes are generated by tracers from objects when passing spans to libdatadog, this is not an error path that's going to happen anyway
There was a problem hiding this comment.
I agree with you here, but it will probably come back and bite us...
this is not an error path that's going to happen anyway
| )?; | ||
| map.serialize_entry("service", service_str)?; | ||
| map.serialize_entry("error", &span.error)?; | ||
| map.serialize_entry("start", &span.start)?; |
There was a problem hiding this comment.
Do we need to apply the unix normalization done for v04 msgpack encoding ? (i.e. Replacing negative by 0)
| if !span_links_seen && !span.span_links.is_empty() { | ||
| if let Some(s) = serialize_span_links(&span.span_links) { | ||
| meta.serialize_entry("_dd.span_links", &s)?; | ||
| } | ||
| } | ||
| if !events_seen && !span.span_events.is_empty() { | ||
| if let Some(s) = serialize_span_events(&span.span_events) { | ||
| meta.serialize_entry("events", &s)?; | ||
| } | ||
| } |
There was a problem hiding this comment.
We should document that span.span_events/span_links are dropped if the field already exists in meta
| /// `https://public-trace-http-intake.logs.datadoghq.com/v1/input`). | ||
| pub endpoint_url: String, | ||
| /// Datadog API key used for the `dd-api-key` header. | ||
| pub api_key: String, |
There was a problem hiding this comment.
nit: Any reason to not use Endpoint to handle the api key ?
gh-worker-dd-devflow-36fce6
Bot
added
mergequeue-status: done
and removed
mergequeue-status: in_progress
labels
…ibdd-data-pipeline, libdd-li... (#2201) # Release proposal for libdd-capabilities-impl, libdd-common, libdd-data-pipeline, libdd-library-config, libdd-remote-config, libdd-sampling, libdd-telemetry, libdd-tinybytes, libdd-trace-utils and their dependencies This PR contains version bumps based on public API changes and commits since last release. ## libdd-capabilities **Next version:** `2.1.0` **Semver bump:** `minor` **Tag:** `libdd-capabilities-v2.1.0` ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-common **Next version:** `5.1.0` **Semver bump:** `minor` **Tag:** `libdd-common-v5.1.0` ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - fix: update rustls-webpki to 0.103.13 (#2187) - fix: update anyhow for unsoundness (#2186) - feat(machine id): Add helpers in ddcommon to fetch the machine UUID l… (#2163) ## libdd-ddsketch **Next version:** `1.1.0` **Semver bump:** `minor` **Tag:** `libdd-ddsketch-v1.1.0` ### Commits - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(ddsketch): add microbenchmarks for add/encode/collapse (#2125) ## libdd-trace-protobuf **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-protobuf-v4.0.0` ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: add whole key cardinality limit (#2158) - feat(remote-config)!: use the proto file from the agent (#2165) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) ## libdd-capabilities-impl **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-capabilities-impl-v3.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(data-pipeline)!: add stdout log trace exporter (#2074) ## libdd-library-config **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-library-config-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - feat(library-config)!: caller-supplied threadlocal schema and extra process-context attributes (#2162) - fix(otel-thread-ctx): put the threadlocal attributes at the right place in the context (#2167) ## libdd-remote-config **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-remote-config-v2.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - refactor(libdd-remote-config)!: hide Target inner properties so they are not leaked (#2182) - feat(remote-config)!: use the proto file from the agent (#2165) - refactor(rc): reexport Endpoint and Tag common types (#2147) ## libdd-trace-normalization **Next version:** `3.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-normalization-v3.0.0` ###⚠️ major bump forced due to: - `libdd-trace-protobuf`: ^3.0.1 → ^4.0.0 ### Commits - feat(data-pipeline)!: CSS Trace Filters (#1985) ## libdd-shared-runtime **Next version:** `2.0.0` **Semver bump:** `major` **Tag:** `libdd-shared-runtime-v2.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ### Commits - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) ## libdd-trace-utils **Next version:** `9.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-utils-v9.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(data-pipeline): add agentless export (#2081) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - test(trace-utils): add V05 msgpack decode microbenchmark (#2127) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - test(trace-utils): add VecMap microbenchmarks (#2126) - chore(stats)!: submit p0 telemetry in stats (#2130) - refactor(change-buffer)!: replace slot index with span_id, fix segment isolation (#2105) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(trace-exporter): add v1 span and its encoder (#2039) - fix(trace-utils): mark decoded span maps as deduped (#2110) - feat(trace-utils)!: change buffer implementation (#2055) - feat(native-spans)!: change buffer foundation (#2046) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - test: fix timeouts on heavily contended scenarios (#2093) ## libdd-telemetry **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-telemetry-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 ### Commits - ci(miri): skip slow miri tests (#2188) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) ## libdd-trace-obfuscation **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-obfuscation-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - refactor(clippy): prefer core and alloc imports (#2196) - ci(miri): skip slow miri tests (#2188) - fix: update anyhow for unsoundness (#2186) ## libdd-trace-stats **Next version:** `6.0.0` **Semver bump:** `major` **Tag:** `libdd-trace-stats-v6.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - chore!: update protobufs to be in sync with datadog-agent (#2180) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) ## libdd-data-pipeline **Next version:** `7.0.0` **Semver bump:** `major` **Tag:** `libdd-data-pipeline-v7.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-shared-runtime`: ^1.0.0 → ^2.0.0 - `libdd-telemetry`: ^5.0.1 → ^6.0.0 - `libdd-trace-protobuf`: ^3.0.2 → ^4.0.0 - `libdd-trace-stats`: ^5.0.0 → ^6.0.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 ### Commits - feat(trace_exporter): enable telemetry in stats exporter (#2160) - refactor(libdd-telemetry)!: avoid leaking libdd-common types in the public API (#2152) - feat(stats): emit canonical gRPC status name for OTLP rpc.response.status_code (#2183) - feat(data-pipeline): add agentless export (#2081) - feat(stats)!: send telemetry for cardinality limits (#2159) - feat(stats)!: add whole key cardinality limit (#2158) - fix(trace-stats)!: add grpc_method to aggregation key (#2151) - feat(data-pipeline)!: add stdout log trace exporter (#2074) - feat(shared-runtime)!: SharedRuntime Borrowed & Owned mode (#2061) - feat(data-pipeline): OTLP HTTP/protobuf trace export (#2115) - feat(otlp)!: Export OTLP spans with attribute-level OTel compatibility (#2091) - feat(data-pipeline)!: export client-computed span stats as OTLP trace metrics (#2067) - chore(stats)!: submit p0 telemetry in stats (#2130) - feat(data-pipeline)!: CSS Trace Filters (#1985) - feat(shared-runtime)!: use weak waker in trigger [APMSP-3371] (#2050) - refactor(span)!: use VecMap for `meta`, `metrics` and `meta_struct` for v04 spans (#2043) - feat(stats)!: add endpoint gating to client-side stats [APMSP-3361] (#2040) ## libdd-dogstatsd-client **Next version:** `4.0.0` **Semver bump:** `major` **Tag:** `libdd-dogstatsd-client-v4.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.1.0 → ^5.1.0 ## libdd-sampling **Next version:** `5.0.0` **Semver bump:** `major` **Tag:** `libdd-sampling-v5.0.0` ###⚠️ major bump forced due to: - `libdd-common`: ^4.2.0 → ^5.1.0 - `libdd-trace-utils`: ^8.0.0 → ^9.0.0 [APMSP-3371]: https://datadoghq.atlassian.net/browse/APMSP-3371?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: iunanua <[email protected]>
# Motivation Send traces to the agentless endpoint to replace the JS and python exporters # What changes * Additional JSON agentless encoder * Additional configuration (agentless enablement, endpoint, API key) * Agentless traces need top level computation, but not dropping spans Co-authored-by: paul.legranddescloizeaux <[email protected]> Signed-off-by: Taegyun Kim <[email protected]>
7 participants
Motivation
Send traces to the agentless endpoint to replace the JS and python exporters
What changes