-
Notifications
You must be signed in to change notification settings - Fork 294
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.
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
Fix iast metrics memory leak #4185
Conversation
Overall package sizeSelf size: 6.24 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4185 +/- ##
=======================================
Coverage 85.22% 85.22%
=======================================
Files 247 247
Lines 10948 10954 +6
Branches 33 33
=======================================
+ Hits 9330 9336 +6
Misses 1618 1618 ☔ View full report in Codecov by Sentry. |
44e9438
to
e69f57e
Compare
BenchmarksBenchmark execution time: 2024-03-26 09:48:34 Comparing candidate commit f0b049d in PR branch Found 2 performance improvements and 0 performance regressions! Performance is the same for 260 metrics, 4 unstable metrics. scenario:plugin-graphql-with-depth-and-collapse-on-18
scenario:plugin-graphql-with-depth-off-18
|
2f63256
to
9ca351a
Compare
expect(namespace.iastMetrics.size).to.be.eq(0) | ||
expect(namespace.metrics.size).to.be.eq(0) | ||
expect(namespace.distributions.size).to.be.eq(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to.be.equal(
is more used than eq
expect(namespace.iastMetrics.size).to.be.eq(0) | |
expect(namespace.metrics.size).to.be.eq(0) | |
expect(namespace.distributions.size).to.be.eq(0) | |
expect(namespace.iastMetrics.size).to.be.equal(0) | |
expect(namespace.metrics.size).to.be.equal(0) | |
expect(namespace.distributions.size).to.be.equal(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked yesterday about changing metrics = new Map()
in line 64 by metrics = new WeakMap()
to be 100% sure that the map is not going to increase to infinity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot use a WeakMap
because:
- tags are cached so they will never be collected by GC
- i need to iterate the map values
Instead, I set a max size for the Map
and check the size to ensure it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The max size should never be reached but just in case...
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
* Do not invoke globalNamespace.count when merging request metrics * Use a WeakMap and minor refactor * Do not filter out a 'lib_language' tag that is no longer used * Use iast metrics when merge instead namespace metrics * Set a max size cache for metric tags * Use .equal * warn via telemetry and clear the cache if max is reached
What does this PR do?
Do not invoke
globalNamespace.count
when merging request metrics.There are two kind of metrics collected by the iast: global and request metrics. In both cases
IastNamespace
is used.IastNamespace
overridesNamespace.count
to cache metrics and uses the metric tags as the cache key.When finalizing the request, stored request metrics are merged into
globalNamespace
in order to be sent via telemetry.And as
globalNamespace
never has a request metric cached it stores the metric automatically causing a leak.Motivation
Fix iast metrics memory leak introduced in #4017
Plugin Checklist
Additional Notes
Security
Datadog employees:
@DataDog/security-design-and-guidance
.Unsure? Have a question? Request a review!