Dual shipping metrics support#704
Merged
shreyamalpani merged 9 commits intomainfrom Jun 11, 2025
Merged
Conversation
Contributor
|
Can you rebase this with main? it's got a bunch of old commits which landed there a while ago |
shreyamalpani
force-pushed
the
shreya.malpani/dual-ship-metrics-new
branch
from
cf7be12 to
d05296e
Compare
shreyamalpani
force-pushed
the
shreya.malpani/dual-ship-metrics-new
branch
from
d05296e to
e09063e
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds support for dual shipping metrics by introducing an additional_endpoints config field (from YAML or DD_ADDITIONAL_ENDPOINTS), wiring it into the EnvConfig merge logic, and updating component revisions.
- Deserialize and test
additional_endpointsin both YAML and env contexts - Merge
additional_endpointsintoEnvConfigwhen not provided via env - Bump
serverless-componentsrevisions; accidental license removal inLICENSE-3rdparty.yml
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| bottlecap/src/config/yaml.rs | Added additional_endpoints field and YAML parsing test |
| bottlecap/src/config/mod.rs | Merge logic for additional_endpoints into runtime config |
| bottlecap/src/config/env.rs | deserialize_additional_endpoints function, struct field, and tests |
| bottlecap/LICENSE-3rdparty.yml | Removed third-party license block for datadog-fips |
| bottlecap/Cargo.toml | Updated serverless-components git revisions |
Comments suppressed due to low confidence (4)
bottlecap/LICENSE-3rdparty.yml:4239
- The license block for
datadog-fipswas removed, which may violate third-party license compliance. Please restore the removed Apache-2.0 license text.
- Licensed under the Apache License, Version 2.0 (the "License");
bottlecap/src/config/mod.rs:249
- There’s no test for the merge behavior when both env and YAML provide
additional_endpoints. Consider adding a unit test to validate thatEnvConfigpicks the correct source.
if config.additional_endpoints.is_empty() {
bottlecap/src/config/env.rs:4
- The function
deserialize_additional_endpointsrelies onserde_json::Valuebut it isn’t imported; adduse serde_json::Value;so the code will compile.
use tracing::error;
bottlecap/src/config/env.rs:292
- The signature of
deserialize_additional_endpointsexpects aDeserializer, but in tests you’re passing aValuedirectly. Consider refactoring the function to accept aValueor use a properDeserializer(e.g.,serde_json::value::Deserializer) in tests.
let result = deserialize_additional_endpoints(input).unwrap();
duncanista
reviewed
Jun 10, 2025
duncanista
reviewed
Jun 10, 2025
| metric_flush_handles: FuturesOrdered<JoinHandle<MetricsRetryBatch>>, | ||
| } | ||
|
|
||
| struct MetricsRetryBatch { |
Contributor
There was a problem hiding this comment.
Eventually we should move this to a metrics owned folder
duncanista
reviewed
Jun 10, 2025
duncanista
reviewed
Jun 10, 2025
| async fn blocking_flush_all( | ||
| logs_flusher: &LogsFlusher, | ||
| metrics_flusher: &mut MetricsFlusher, | ||
| metrics_flusher: &mut [MetricsFlusher], |
Contributor
There was a problem hiding this comment.
Can we be more explicit here with Vec<...>? Just for readability
Contributor
Author
There was a problem hiding this comment.
I initially had this as Vec<...> but clippy doesn't like it
duncanista
reviewed
Jun 10, 2025
duncanista
reviewed
Jun 10, 2025
| tokio::join!( | ||
| logs_flusher.flush(None), | ||
| metrics_flusher.flush(), | ||
| futures::future::join_all(metrics_futures), |
duncanista
reviewed
Jun 10, 2025
duncanista
reviewed
Jun 10, 2025
Comment on lines
+916
to
+932
| for (endpoint_url, api_keys) in &config.additional_endpoints { | ||
| let dd_url = DdUrl::new(endpoint_url.clone()).expect("can't parse additional endpoint URL"); | ||
| let prefix_override = MetricsIntakeUrlPrefixOverride::maybe_new(Some(dd_url), None); | ||
| let metrics_intake_url = MetricsIntakeUrlPrefix::new(None, prefix_override) | ||
| .expect("can't parse additional endpoint URL"); | ||
|
|
||
| // Create a flusher for each endpoint URL and API key pair | ||
| for api_key in api_keys { | ||
| let additional_flusher_config = MetricsFlusherConfig { | ||
| api_key: api_key.clone(), | ||
| aggregator: metrics_aggr.clone(), | ||
| metrics_intake_url_prefix: metrics_intake_url.clone(), | ||
| https_proxy: config.https_proxy.clone(), | ||
| timeout: Duration::from_secs(config.flush_timeout), | ||
| retry_strategy: DsdRetryStrategy::Immediate(3), | ||
| }; | ||
| flushers.push(MetricsFlusher::new(additional_flusher_config)); |
Contributor
There was a problem hiding this comment.
Wondering if this logic should live in main, not something to address now
Contributor
Author
There was a problem hiding this comment.
I was thinking about this too, I can work on refactoring this when implementing dual shipping for log/traces if that could live together?
duncanista
reviewed
Jun 10, 2025
astuyve
reviewed
Jun 10, 2025
astuyve
reviewed
Jun 10, 2025
| flushers.push(MetricsFlusher::new(flusher_config)); | ||
|
|
||
| for (endpoint_url, api_keys) in &config.additional_endpoints { | ||
| let dd_url = DdUrl::new(endpoint_url.clone()).expect("can't parse additional endpoint URL"); |
Contributor
There was a problem hiding this comment.
There are a few more expects here. While previously we've tolerated them, can we update this so the program doesn't crash if the user provides an invalid site? We can log a fatal error and then replace it with datadoghq.com maybe?
Contributor
Author
There was a problem hiding this comment.
Yes sounds good
Contributor
Author
There was a problem hiding this comment.
Fixed this, tested that it falls back and ships data as expected
duncanpharvey
pushed a commit
that referenced
this pull request
Mar 10, 2026
Adds support for dual shipping metrics to endpoints configured using the `additional_endpoints` YAML or `DD_ADDITIONAL_ENDPOINTS` env var config. For each configured endpoint/API key combination, we now create a separate `MetricsFlusher` to flush the same batch of metrics to multiple endpoints in parallel. Also, updates the retry logic to retry flushing for the specific flusher that encountered an error. Tested dual shipping metrics to 2 additional orgs/endpoints including eu1. Depends on dogstatsd changes: DataDog/serverless-components#20
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
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 support for dual shipping metrics to endpoints configured using the
additional_endpointsYAML orDD_ADDITIONAL_ENDPOINTSenv var config.For each configured endpoint/API key combination, we now create a separate
MetricsFlusherto flush the same batch of metrics to multiple endpoints in parallel. Also, updates the retry logic to retry flushing for the specific flusher that encountered an error.Tested dual shipping metrics to 2 additional orgs/endpoints including eu1.
Depends on dogstatsd changes: DataDog/serverless-components#20
SVLS-6884