Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(datadog_metrics sink): support and migrate to the v2 series API endpoint #18761

Merged
merged 26 commits into from
Oct 12, 2023
Merged

chore(datadog_metrics sink): support and migrate to the v2 series API endpoint #18761

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
d8891fd
add scaffolding for the differentiation of series API versions
neuronull Oct 2, 2023
91ca255
fix(datadog_metrics sink): fix the integration tests which weren't ac…
neuronull Oct 3, 2023
9afbd83
fix workflows
neuronull Oct 3, 2023
cc9cc2d
clippy
neuronull Oct 3, 2023
597305d
fix filter for traces
neuronull Oct 3, 2023
3f9cd72
add first pass
neuronull Oct 3, 2023
4e204c5
Merge branch 'neuronull/fix_datadog_metrics_sink_integration_tests' i…
neuronull Oct 3, 2023
3a96f18
add testing coverage
neuronull Oct 3, 2023
9923219
cargo.lock
neuronull Oct 3, 2023
4523fcd
reduce duplicated code
neuronull Oct 4, 2023
a87bf0c
cleanup
neuronull Oct 4, 2023
376da18
clippy
neuronull Oct 4, 2023
715cf22
Merge branch 'master' into neuronull/fix_datadog_metrics_sink_integra…
neuronull Oct 4, 2023
7117d98
Merge branch 'neuronull/fix_datadog_metrics_sink_integration_tests' i…
neuronull Oct 4, 2023
5912324
Merge branch 'master' into neuronull/sink_datadog_metrics_v2_series_e…
neuronull Oct 5, 2023
c95a326
feedback ds: remove check for sort by name
neuronull Oct 5, 2023
d73cfe0
feedback ds: extend unit tests for v2
neuronull Oct 5, 2023
c9c0fbc
feedback ds: extend the int test coverage
neuronull Oct 5, 2023
2ac5bbf
Revert "feedback ds: remove check for sort by name"
neuronull Oct 5, 2023
cb92dc5
add explicit sort check
neuronull Oct 5, 2023
96ef122
add env var for v1 support
neuronull Oct 5, 2023
2de3617
check events
neuronull Oct 5, 2023
b9181e0
add note in deprecations
neuronull Oct 6, 2023
4920c7c
Merge branch 'master' into neuronull/sink_datadog_metrics_v2_series_e…
neuronull Oct 10, 2023
d513eaf
Merge branch 'master' into neuronull/sink_datadog_metrics_v2_series_e…
neuronull Oct 10, 2023
244b9ed
remove dead code allow
neuronull Oct 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions .github/workflows/changes.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,14 @@ on:
value: ${{ jobs.int_tests.outputs.clickhouse }}
databend:
value: ${{ jobs.int_tests.outputs.databend }}
datadog:
value: ${{ jobs.int_tests.outputs.datadog }}
datadog-agent:
value: ${{ jobs.int_tests.outputs.datadog-agent }}
datadog-logs:
value: ${{ jobs.int_tests.outputs.datadog-logs }}
datadog-metrics:
value: ${{ jobs.int_tests.outputs.datadog-metrics }}
datadog-traces:
value: ${{ jobs.int_tests.outputs.datadog-traces }}
dnstap:
value: ${{ jobs.int_tests.outputs.dnstap }}
docker-logs:
Expand Down Expand Up @@ -189,7 +195,10 @@ jobs:
azure: ${{ steps.filter.outputs.azure }}
clickhouse: ${{ steps.filter.outputs.clickhouse }}
databend: ${{ steps.filter.outputs.databend }}
datadog: ${{ steps.filter.outputs.datadog }}
datadog-agent: ${{ steps.filter.outputs.datadog-agent }}
datadog-logs: ${{ steps.filter.outputs.datadog-logs }}
datadog-metrics: ${{ steps.filter.outputs.datadog-metrics }}
datadog-traces: ${{ steps.filter.outputs.datadog-traces }}
dnstap: ${{ steps.filter.outputs.dnstap }}
docker-logs: ${{ steps.filter.outputs.docker-logs }}
elasticsearch: ${{ steps.filter.outputs.elasticsearch }}
Expand Down
13 changes: 8 additions & 5 deletions .github/workflows/integration.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,10 @@ jobs:
|| needs.changes.outputs.azure == 'true'
|| needs.changes.outputs.clickhouse == 'true'
|| needs.changes.outputs.databend == 'true'
|| needs.changes.outputs.datadog == 'true'
|| needs.changes.outputs.datadog-agent == 'true'
|| needs.changes.outputs.datadog-logs == 'true'
|| needs.changes.outputs.datadog-metrics == 'true'
|| needs.changes.outputs.datadog-traces == 'true'
|| needs.changes.outputs.dnstap == 'true'
|| needs.changes.outputs.docker-logs == 'true'
|| needs.changes.outputs.elasticsearch == 'true'
Expand Down Expand Up @@ -166,7 +169,7 @@ jobs:
max_attempts: 3
command: bash scripts/ci-integration-test.sh databend

- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog == 'true') &&
- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog-agent == 'true') &&
(github.event_name != 'pull_request' || env.PR_HAS_ACCESS_TO_SECRETS == 'true')
name: datadog-agent
uses: nick-fields/retry@v2
Expand All @@ -175,7 +178,7 @@ jobs:
max_attempts: 3
command: bash scripts/ci-integration-test.sh datadog-agent

- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog == 'true') &&
- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog-logs == 'true') &&
(github.event_name != 'pull_request' || env.PR_HAS_ACCESS_TO_SECRETS == 'true')
name: datadog-logs
uses: nick-fields/retry@v2
Expand All @@ -184,7 +187,7 @@ jobs:
max_attempts: 3
command: bash scripts/ci-integration-test.sh datadog-logs

- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog == 'true') &&
- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog-metrics == 'true') &&
(github.event_name != 'pull_request' || env.PR_HAS_ACCESS_TO_SECRETS == 'true')
name: datadog-metrics
uses: nick-fields/retry@v2
Expand All @@ -193,7 +196,7 @@ jobs:
max_attempts: 3
command: bash scripts/ci-integration-test.sh datadog-metrics

- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog == 'true') &&
- if: (github.event_name == 'merge_group' || needs.changes.outputs.all-int == 'true' || needs.changes.outputs.datadog-traces == 'true') &&
(github.event_name != 'pull_request' || env.PR_HAS_ACCESS_TO_SECRETS == 'true')
name: datadog-traces
uses: nick-fields/retry@v2
Expand Down
7 changes: 7 additions & 0 deletions lib/vector-core/src/event/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,13 @@ impl Event {
self
}

/// Sets the `source_type` in the event metadata to the provided value.
#[must_use]
pub fn with_source_type(mut self, source_type: &'static str) -> Self {
self.metadata_mut().set_source_type(source_type);
self
}

/// Sets the `upstream_id` in the event metadata to the provided value.
#[must_use]
pub fn with_upstream_id(mut self, upstream_id: Arc<OutputId>) -> Self {
Expand Down
2 changes: 1 addition & 1 deletion scripts/integration/datadog-metrics/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
features:
- datadog-metrics-integration-tests

test_filter: '::datadog::metrics::'
test_filter: '::datadog::metrics::integration_tests'

runner:
env:
Expand Down
2 changes: 1 addition & 1 deletion scripts/integration/datadog-traces/test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,6 @@ matrix:
paths:
- "src/common/datadog.rs"
- "src/internal_events/datadog_*"
- "src/sinks/datadog/**"
- "src/sinks/datadog/traces/**"
- "src/sinks/util/**"
- "scripts/integration/datadog-traces/**"
45 changes: 38 additions & 7 deletions src/sinks/datadog/metrics/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,27 +42,58 @@ impl SinkBatchSettings for DatadogMetricsDefaultBatchSettings {
const TIMEOUT_SECS: f64 = 2.0;
}

pub(super) const SERIES_V1_PATH: &str = "/api/v1/series";
pub(super) const SERIES_V2_PATH: &str = "/api/v2/series";
pub(super) const SKETCHES_PATH: &str = "/api/beta/sketches";

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
#[allow(dead_code)]
neuronull marked this conversation as resolved.
Show resolved Hide resolved
pub enum SeriesApiVersion {
V1,
V2,
}

impl SeriesApiVersion {
pub const fn get_path(self) -> &'static str {
match self {
Self::V1 => SERIES_V1_PATH,
Self::V2 => SERIES_V2_PATH,
}
}
}

impl Default for SeriesApiVersion {
fn default() -> Self {
Self::V2
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the goal with all the configurable stuff here? Seems like the intent to move wholesale to the v2 API, since there's nothing exposed to allow staying on/switching back to the v1 API, at least from the user's perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was going to call this out when I finalized the PR, but I recall chatting with @jszwedko about this (the history of the convo is gone though) . I don't remember the details but I believe the conclusion was we wanted to keep the v1 logic if it wasn't too difficult to do so. We had discussed allowing the api version to be configurable somehow, but felt that wasn't necessary.

So I think it might boil down to us wanting to be able to flip a switch to go back to V1 if there were issues with our rollout of V2.

Another idea I had thrown out was having a non publicized backdoor essentially, that would allow a user to configure to use the v1. Its possible we could do that with the endpoint configuration I think. We'd just have to have some uglier logic in the parsing of that for this sink, since it isn't supposed to specify the path, just the host/port.

TLDR ... Its open for discussion 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I see including the fallback as a risk mitigation measure so that if it does break things, users have a workaround until we fix it. We can weigh the risk against the level of effort to maintain the v1 implementation though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

users have a workaround until we fix it

In it's current state, users would have to edit the code and recompile in order to take advantage of that. But, my suggestion here:

Another idea I had thrown out was having a non publicized backdoor essentially, that would allow a user to configure to use the v1. Its possible we could do that with the endpoint configuration I think. We'd just have to have some uglier logic in the parsing of that for this sink, since it isn't supposed to specify the path, just the host/port.

, would allow us to give users a workaround that would just entail editing their config. If we did this, could tag it with a TODO and followup GH issue to remove the backdoor and the v1 support altogether, once it had enough soak time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we'd like to expose this to the end user temporarily as we roll out this change, an unpublicized environment variable could be a good option. Something like VECTOR_TEMP_USE_DD_METRICS_SERIES_V1_API that clearly denotes that it is a temporary env variable that will be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can get behind a temporary env var.

Yeah, I see including the fallback as a risk mitigation measure so that if it does break things, users have a workaround until we fix it. We can weigh the risk against the level of effort to maintain the v1 implementation though.

To clarify- IMO I think we would only want to maintain the v1 implementation for a full release, maybe two. But after its clear there aren't issues, we'd remove it. In my view the maintenance burden of supporting the v1 implementation for a release or two should be very small if not zero.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the env var in the latest changes and made code comments regarding the temporary support of V1. Since this isn't a user-facing deprecation/change, I dont think we need to follow the formal DEPRECATION policy if I'm not mistaken

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Which I validated works by injecting it in the vdev env vars and seeing the debug logs from the integration tests)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't a user-facing deprecation/change, I dont think we need to follow the formal DEPRECATION policy if I'm not mistaken

As discussed, I noted this in the DEPRECATIONS file but am holding off on a mention to the upgrade guide as its not a user facing change and is lowish risk

/// Various metric type-specific API types.
///
/// Each of these corresponds to a specific request path when making a request to the agent API.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum DatadogMetricsEndpoint {
Series,
Series(SeriesApiVersion),
Sketches,
}

impl DatadogMetricsEndpoint {
/// Gets the content type associated with the specific encoder for a given metric endpoint.
pub const fn content_type(self) -> &'static str {
match self {
DatadogMetricsEndpoint::Series => "application/json",
DatadogMetricsEndpoint::Sketches => "application/x-protobuf",
Self::Series(SeriesApiVersion::V1) => "application/json",
Self::Sketches | Self::Series(SeriesApiVersion::V2) => "application/x-protobuf",
}
}

// Gets whether or not this is a series endpoint.
pub const fn is_series(self) -> bool {
matches!(self, Self::Series)
matches!(self, Self::Series { .. })
}

// Creates an instance of the `Series` variant with the default API version.
pub fn series() -> Self {
Self::Series(SeriesApiVersion::default())
}
}

Expand All @@ -84,7 +115,7 @@ impl DatadogMetricsEndpointConfiguration {
/// Gets the URI for the given Datadog metrics endpoint.
pub fn get_uri_for_endpoint(&self, endpoint: DatadogMetricsEndpoint) -> Uri {
match endpoint {
DatadogMetricsEndpoint::Series => self.series_endpoint.clone(),
DatadogMetricsEndpoint::Series { .. } => self.series_endpoint.clone(),
DatadogMetricsEndpoint::Sketches => self.sketches_endpoint.clone(),
}
}
Expand Down Expand Up @@ -169,8 +200,8 @@ impl DatadogMetricsConfig {
&self,
) -> crate::Result<DatadogMetricsEndpointConfiguration> {
let base_uri = self.get_base_agent_endpoint();
let series_endpoint = build_uri(&base_uri, "/api/v1/series")?;
let sketches_endpoint = build_uri(&base_uri, "/api/beta/sketches")?;
let series_endpoint = build_uri(&base_uri, SeriesApiVersion::default().get_path())?;
let sketches_endpoint = build_uri(&base_uri, SKETCHES_PATH)?;

Ok(DatadogMetricsEndpointConfiguration::new(
series_endpoint,
Expand Down
Loading