support v3 in fakeintake#51150
Conversation
|
@codex review |
dd-octo-sts
Bot
added
internal
|
![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: 69873fff60
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "/api/v1/check_run": getCheckRunPayLoadJSON, | ||
| "/api/v1/connections": getConnectionsPayLoadProtobuf, | ||
| "/api/beta/sketches": getSketchPayloadProtobuf, | ||
| "/api/intake/metrics/v3/series": getMetricV3SeriesPayload, |
There was a problem hiding this comment.
This change enables serializer_experimental_use_v3_api.sketches.endpoints (host and Helm paths), so when V3 is turned on the agent sends sketch payloads to /api/intake/metrics/v3/sketches; however fakeintake only adds a parser for /api/intake/metrics/v3/series here, and FilterSketches() still reads /api/beta/sketches. In V3 mode, distribution/sketch assertions will therefore silently see no data even though payloads are being sent, which breaks end-to-end metric coverage for sketches.
Useful? React with 👍 / 👎.
Files inventory check summaryFile checks results against ancestor 5d847224: Results for datadog-agent_7.81.0~devel.git.105.ca9624c.pipeline.114521494-1_amd64.deb:No change detected |
atanzu
force-pushed
the
mark.kirichenko/support-v3-in-fakeintake
branch
from
69873ff to
46d4a45
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46d4a4504b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "/api/v1/check_run": getCheckRunPayLoadJSON, | ||
| "/api/v1/connections": getConnectionsPayLoadProtobuf, | ||
| "/api/beta/sketches": getSketchPayloadProtobuf, | ||
| "/api/intake/metrics/v3/series": getMetricV3SeriesPayload, |
There was a problem hiding this comment.
This map adds /api/intake/metrics/v3/series but leaves out /api/intake/metrics/v3/sketches, even though WithV3MetricsEnabled now configures both series and sketches V3 endpoints. In that configuration, sketch payloads are sent to the V3 sketches route (comp/forwarder/defaultforwarder/endpoints/endpoints.go) and fakeintake won’t decode them (and FilterSketches still reads /api/beta/sketches), so sketch-based assertions under V3 mode silently miss data. Either add V3-sketch parsing/client support or stop enabling the sketches V3 endpoint until that support exists.
Useful? React with 👍 / 👎.
atanzu
changed the title
atanzu
force-pushed
the
mark.kirichenko/support-v3-in-fakeintake
branch
from
46d4a45 to
b2b4d07
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2b4d07492
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "/api/v1/check_run": getCheckRunPayLoadJSON, | ||
| "/api/v1/connections": getConnectionsPayLoadProtobuf, | ||
| "/api/beta/sketches": getSketchPayloadProtobuf, | ||
| "/api/intake/metrics/v3/series": getMetricV3SeriesPayload, |
There was a problem hiding this comment.
When V3 is enabled, the new config paths set both series and sketches V3 endpoints to fakeintake, so sketch payloads are sent to /api/intake/metrics/v3/sketches. This parser map only adds /api/intake/metrics/v3/series, so V3 sketch payloads remain unparsed and FilterSketches() (which still reads /api/beta/sketches) will silently miss them. Any V3-enabled tests asserting distributions/sketches can therefore fail or give false negatives.
Useful? React with 👍 / 👎.
atanzu
force-pushed
the
mark.kirichenko/support-v3-in-fakeintake
branch
from
b2b4d07 to
8995a94
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8995a94b17
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sketches: | ||
| endpoints: | ||
| - %[3]s://%[1]s:%[2]d |
There was a problem hiding this comment.
This option now enables serializer_experimental_use_v3_api.sketches.endpoints, which redirects sketch/distribution traffic to /api/intake/metrics/v3/sketches, but fakeintake still has no corresponding v3-sketch retrieval path (FilterSketches still queries /api/beta/sketches, and there is no /api/intake/metrics/v3/sketches handler in serverstore/parser.go). As soon as a v3-enabled test needs to assert sketch metrics, it will get empty results despite sketches being sent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We can leave sketches out entirely from the config, or separate them from series so we can enable it without sketches.
atanzu
force-pushed
the
mark.kirichenko/support-v3-in-fakeintake
branch
from
8995a94 to
ca9624c
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca9624c4fe
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sketches: | ||
| endpoints: | ||
| - %[3]s://%[1]s:%[2]d |
There was a problem hiding this comment.
This now configures serializer_experimental_use_v3_api.sketches.endpoints, so sketch/distribution payloads are sent to /api/intake/metrics/v3/sketches, but fakeintake still only exposes sketch parsing/querying on /api/beta/sketches (FilterSketches/GetSketchNames in test/fakeintake/client/client.go). In suites that enable V3 and assert on sketch metrics, those assertions will incorrectly see no data because sketches have been rerouted to an unsupported endpoint.
Useful? React with 👍 / 👎.
atanzu
force-pushed
the
mark.kirichenko/support-v3-in-fakeintake
branch
from
ca9624c to
3655bb2
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3655bb2f3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| sketches: | ||
| endpoints: | ||
| - %[3]s://%[1]s:%[2]d |
There was a problem hiding this comment.
WithV3MetricsEnabled also rewrites sketch traffic to V3, but this change set only adds fakeintake support for /api/intake/metrics/v3/series (new parser/client path) and adds no parser/client support for /api/intake/metrics/v3/sketches. In E2E runs that emit distribution/sketch metrics with V3 enabled, sketch payloads are sent to an endpoint fakeintake cannot decode/query, so FilterSketches-style assertions will silently see no data and tests can miss or misreport regressions.
Useful? React with 👍 / 👎.
atanzu
added
the
qa/done
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3655bb2f3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| envVars := pulumi.Map{ | ||
| "DD_SERIALIZER_EXPERIMENTAL_USE_V3_API_SERIES_ENDPOINTS": pulumi.Sprintf("%s", p.intakeURL), | ||
| "DD_SERIALIZER_EXPERIMENTAL_USE_V3_API_SKETCHES_ENDPOINTS": pulumi.Sprintf("%s", p.intakeURL), |
There was a problem hiding this comment.
WithV3MetricsEnabled now sets DD_SERIALIZER_EXPERIMENTAL_USE_V3_API_SKETCHES_ENDPOINTS, which moves distribution/sketch traffic to /api/intake/metrics/v3/sketches, but this change only adds V3 handling for /api/intake/metrics/v3/series (test/fakeintake/server/serverstore/parser.go) and the client still reads sketches from /api/beta/sketches (test/fakeintake/client/client.go). In V3-enabled E2E runs that assert sketch/distribution metrics, fakeintake will silently return no sketches even though the agent is sending them, causing false negatives.
Useful? React with 👍 / 👎.
| sketches: | ||
| endpoints: | ||
| - %[3]s://%[1]s:%[2]d |
There was a problem hiding this comment.
We can leave sketches out entirely from the config, or separate them from series so we can enable it without sketches.
| if p.intakeHostname == nil || p.intakePort == nil || p.intakeScheme == nil { | ||
| return fmt.Errorf("WithV3MetricsEnabled must be called after WithFakeintake or WithIntakeHostname") | ||
| } | ||
| v3Config := pulumi.Sprintf(`serializer_experimental_use_v3_api: |
There was a problem hiding this comment.
This is my personal preference, but I try to avoid plain text templating like this. I'd make a struct and serialize it with proper serializer, just to avoid potential issues with quoting strings or counting spaces.
| // Store so that WithV3MetricsEnabled (applied after) can build V3 endpoints config. | ||
| p.intakeScheme = scheme | ||
| p.intakeHostname = hostname | ||
| p.intakePort = port |
There was a problem hiding this comment.
With this, we format the url twice. Would it make sense to construct it here, stash it and use the complete string in two places?
| return nil, fmt.Errorf("v3 next sketch point: %w", err) | ||
| } | ||
| } | ||
| continue |
There was a problem hiding this comment.
We should return an error if a sketch is encountered in a series payload, it would be a serious bug in the agent.
| case encodingDeflate: | ||
| rc, err = zlib.NewReader(bytes.NewReader(payload)) | ||
| case encodingZstd: | ||
| rc = zstd.NewReader(bytes.NewReader(payload)) |
There was a problem hiding this comment.
This should work with zstd with the latest head of DataDog/zstd (v1.5.8-0.20250922095318-5c504fb5d923).
There was a problem hiding this comment.
The problem is, I don't see a stable 1.5.8 tag. Would that be OK to have that specific pinned version?
There was a problem hiding this comment.
I'll follow up on the release, but yes, it should be OK to pin pre-release version.
There was a problem hiding this comment.
It was released as 1.5.7+patch1, +patch is how we version our changes on top of the upstream.
| // MetricSeriesV3 represents a single time series decoded from a V3 metrics intake payload | ||
| // (/api/intake/metrics/v3/series). The V3 format is a column-oriented protobuf encoding | ||
| // described in pkg/proto/datadog/dogstatsdhttp/payload.proto. | ||
| type MetricSeriesV3 struct { |
There was a problem hiding this comment.
Would it make sense to use the old metricAggregator for v3 as well? V3 is expected to be equivalent (at least until we turn on sketches), and should be transparent change. Ideally, the existing tests that currently use v2 protocol should upgrade when we enable v3 by default (and it would be nice if we can run the same suite in parallel for both versions). WDYT?
There was a problem hiding this comment.
My initial intent was to make this check as explicit as possible. You are right in terms of usage of a single metricAggregator and its transparency: after the flip the modified fakeintake shall work as is. However, for now we still need to have a clear vision between v2 and v3 (and check that we don't have mixed traffic), so I will update a couple of the existing test suites to have a v3 variant.
5 participants
What does this PR do?
Allow us to test V3 payloads with fakeintake.
Motivation
Current e2e test setup doesn't support the V3 payload format. We want to address this gap.
We add V3 metrics intake support, which relies on the V3 parser from dogstatsd-http. We cannot import this module directly, so we use a copy of it.
Describe how you validated your changes
We add new e2e tests for our usual metrics and for OTel data which verify that we can send data to a V3 endpoint instead of V2.
Additional Notes
V3 payload parser is a copy from dogstatsd-http, so we need to update both in parallel in case of any changes.