Skip to content

contrib/envoyproxy(feat): Implement API Sec proxy route sampler#3698

Merged
e-n-0 merged 5 commits intomainfrom
flavien/proxy-route-sampler
Jun 30, 2025
Merged

contrib/envoyproxy(feat): Implement API Sec proxy route sampler#3698
e-n-0 merged 5 commits intomainfrom
flavien/proxy-route-sampler

Conversation

@e-n-0
Copy link
Copy Markdown
Member

@e-n-0 e-n-0 commented Jun 26, 2025 •
edited
Loading

What does this PR do?

This PR add a new API Security route sampler for proxies. It is defined in this RFC.

Using the new release of appsec-internal-go v1.13.0 (PR DataDog/appsec-internal-go#52)

Motivation

Correctly sample the requests for schema generation inside proxy environment.
Currently, the sampler applied is by key on the path (fallback on the path because the route is not available). This PR means to change it.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • New code is free of linting errors. You can check this by running golangci-lint run locally.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@pr-commenter
Copy link
Copy Markdown

pr-commenter Bot commented Jun 26, 2025 •
edited
Loading

Benchmarks

Benchmark execution time: 2025-06-27 15:38:12

Comparing candidate commit 71622f3 in PR branch flavien/proxy-route-sampler with baseline commit fe9272d in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 24 metrics, 0 unstable metrics.

@datadog-datadog-prod-us1
Copy link
Copy Markdown

datadog-datadog-prod-us1 Bot commented Jun 26, 2025 •
edited
Loading

Datadog Report

Branch report: flavien/proxy-route-sampler
Commit report: 25c9c0c
Test service: dd-trace-go

❌ 1 Failed (0 Known Flaky), 5422 Passed, 66 Skipped, 4m 23.29s Total Time

❌ Failed Tests (1)

  • TestMultipleSpanIntegrationTags - github.com/DataDog/dd-trace-go/v2/ddtrace/tracer - Details

    Expand for error 
     Failed
 
 === RUN   TestMultipleSpanIntegrationTags
     metrics_test.go:211: 
         	Error Trace:	/home/runner/work/dd-trace-go/dd-trace-go/ddtrace/tracer/metrics_test.go:211
         	Error:      	Not equal: 
         	            	expected: 10
         	            	actual  : 9
         	Test:       	TestMultipleSpanIntegrationTags
     metrics_test.go:218: 
 ...

@e-n-0 e-n-0 force-pushed the flavien/proxy-route-sampler branch 2 times, most recently from b9edafa to 6ec7e89 Compare June 26, 2025 14:29
@e-n-0 e-n-0 force-pushed the flavien/proxy-route-sampler branch from 6ec7e89 to a973312 Compare June 26, 2025 14:30
@e-n-0 e-n-0 marked this pull request as ready for review June 26, 2025 18:15
@e-n-0 e-n-0 requested a review from a team as a code owner June 26, 2025 18:15
@github-actions github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Jun 26, 2025
RomainMuller
RomainMuller requested changes Jun 27, 2025
View reviewed changes
Comment thread internal/appsec/config/config.go Outdated
Comment thread internal/appsec/config/config.go
Comment thread internal/appsec/waf_test.go
t.Run("rate-limits", func(t *testing.T) {
t.Setenv(config.EnvEnabled, "true")
t.Setenv(internal.EnvAPISecEnabled, "true")
t.Setenv(internal.EnvAPISecProxySampleRate, "1")
Copy link
Copy Markdown
Contributor

@RomainMuller RomainMuller Jun 27, 2025

Choose a reason for hiding this comment

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

Ugh I should've asked for this constant name to mention the unit of the value; because here I have no clue...

I'd appreciate if you can:

  1. Add a comment here mentioning the unit (samples-per-second?)
  2. Maybe make a small PR to appsec-internal-go to document it on the constant if not done already.

And then, depending on the unit -- this may be too low to guarantee it'll always behave right on slow CI hosts... Should probably not be afraid to pump it up ridiculously.

Copy link
Copy Markdown
Member Author

@e-n-0 e-n-0 Jun 27, 2025

Choose a reason for hiding this comment

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

The value of EnvAPISecProxySampleRate is a sample rate by minute. I opened this PR to refine the doc: DataDog/appsec-internal-go#53

I also updated the test to test that after 1 min, the sampler is restaured.

Copy link
Copy Markdown
Contributor

@RomainMuller RomainMuller Jun 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I also updated the test to test that after 1 min, the sampler is restaured.

You sure about that? I don't see it... And you most certainly don't want your test to include a time.Sleep(time.Minute) -- that is a lot of time in CI!

Copy link
Copy Markdown
Member Author

@e-n-0 e-n-0 Jun 27, 2025

Choose a reason for hiding this comment

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

I pushed the commit just after the comment 😄
The thing is that the minute of the sampler is not configurable, it will be always 1 min.

@e-n-0 e-n-0 mentioned this pull request Jun 27, 2025
Merged
RomainMuller pushed a commit to DataDog/appsec-internal-go that referenced this pull request Jun 27, 2025
@e-n-0
Refine documentation for the proxy sample rate (#53)
941568f 
# Changes
Related to the PR DataDog/dd-trace-go#3698, the
documentation of the proxy sample rate has been updated.
@e-n-0 e-n-0 enabled auto-merge (squash) June 27, 2025 15:23
@e-n-0 e-n-0 merged commit 901f1f8 into main Jun 30, 2025
321 of 325 checks passed
@e-n-0 e-n-0 deleted the flavien/proxy-route-sampler branch June 30, 2025 08:52
eliottness pushed a commit that referenced this pull request Jul 9, 2025
@e-n-0 @eliottness
contrib/envoyproxy(feat): Implement API Sec proxy route sampler (#3698)
22dadda
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RomainMuller RomainMuller RomainMuller approved these changes

Assignees

No one assigned

Labels

apm:ecosystem contrib/* related feature requests or bugs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@e-n-0 @RomainMuller