Skip to content

Commit

Permalink
Do not reset IAST concurrent request counter (#7963)
Browse files Browse the repository at this point in the history
  • Loading branch information
smola authored Nov 19, 2024
1 parent 6181783 commit 4dfa404
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 8 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import datadog.trace.api.gateway.RequestContext;
import datadog.trace.api.gateway.RequestContextSlot;
import datadog.trace.api.iast.IastContext;
import datadog.trace.api.telemetry.LogCollector;
import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
import datadog.trace.bootstrap.instrumentation.api.AgentTracer;
import datadog.trace.util.AgentTaskScheduler;
Expand Down Expand Up @@ -127,6 +128,8 @@ private int getAvailableQuote(@Nullable final AgentSpan span) {

class OverheadControllerImpl implements OverheadController {

private static final Logger LOGGER = LoggerFactory.getLogger(OverheadControllerImpl.class);

private static final int RESET_PERIOD_SECONDS = 30;

private final int sampling;
Expand All @@ -141,6 +144,8 @@ class OverheadControllerImpl implements OverheadController {

final AtomicLong cumulativeCounter;

private volatile long lastAcquiredTimestamp = Long.MAX_VALUE;

final OverheadContext globalContext =
new OverheadContext(Config.get().getIastVulnerabilitiesPerRequest());

Expand All @@ -165,7 +170,11 @@ public boolean acquireRequest() {
long newValue = prevValue + sampling;
if (newValue / 100 == prevValue / 100 + 1) {
// Sample request
return availableRequests.acquire();
final boolean acquired = availableRequests.acquire();
if (acquired) {
lastAcquiredTimestamp = System.currentTimeMillis();
}
return acquired;
}
// Skipped by sampling
return false;
Expand Down Expand Up @@ -222,11 +231,22 @@ static NonBlockingSemaphore maxConcurrentRequests(final int max) {
@Override
public void reset() {
globalContext.reset();
// Periodic reset of maximum concurrent requests. This guards us against exhausting concurrent
// requests if some bug led us to lose a request end event. This will lead to periodically
// going above the max concurrent requests. But overall, it should be self-stabilizing. So for
// practical purposes, the max concurrent requests is a hint.
availableRequests.reset();
if (lastAcquiredTimestamp != Long.MAX_VALUE
&& System.currentTimeMillis() - lastAcquiredTimestamp > 1000 * 60 * 60) {
// If the last time a request was acquired is longer than 1h, we check the number of
// available requests. If it
// is zero, we might have lost request end events, leading to IAST not being able to acquire
// new requests.
// We report it to telemetry for further investigation.
if (availableRequests.available() == 0) {
LOGGER.debug(
LogCollector.SEND_TELEMETRY,
"IAST cannot acquire new requests, end of request events might be missing.");
// Once starved, do not report this again, unless this is recovered and then starved
// again.
lastAcquiredTimestamp = Long.MAX_VALUE;
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,7 @@ class OverheadControllerTest extends DDSpecification {
executorService?.shutdown()
}

def 'acquireRequest works for max concurrent request per reset'() {
def 'acquireRequest never exceeds max concurrent requests even after reset'() {
setup:
def taskSchedler = Stub(AgentTaskScheduler)
injectSysConfig("dd.iast.request-sampling", "100")
Expand All @@ -270,7 +270,7 @@ class OverheadControllerTest extends DDSpecification {
lastAcquired = overheadController.acquireRequest()

then:
acquiredValues.every { it == true }
acquiredValues.every { it == false }
!lastAcquired

when:
Expand Down

0 comments on commit 4dfa404

Please sign in to comment.