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.

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

[DI] Drop snapshot if JSON payload is too large #4818

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

watson
Copy link
Collaborator

@watson watson commented Oct 23, 2024

The log track has a 1MB limit of the JSON payload. The client is not notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated algorithm will be implemented that reduces the size of the snapshot instead of removing it completely.

@watson watson requested review from a team as code owners October 23, 2024 19:08
@watson watson self-assigned this Oct 23, 2024
Copy link
Collaborator Author

watson commented Oct 23, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @watson and the rest of your teammates on Graphite Graphite

Copy link

github-actions bot commented Oct 23, 2024

Overall package size

Self size: 7.65 MB
Deduped: 64.51 MB
No deduping: 64.85 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.2.1 | 19.18 MB | 19.19 MB | | @datadog/native-iast-taint-tracking | 3.2.0 | 13.9 MB | 13.91 MB | | @datadog/pprof | 5.4.1 | 9.76 MB | 10.13 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.5.0 | 2.51 MB | 2.65 MB | | @opentelemetry/core | 1.14.0 | 872.87 kB | 1.47 MB | | @datadog/native-metrics | 2.0.0 | 898.77 kB | 1.3 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.11.2 | 112.74 kB | 826.22 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.0 | 109.9 kB | 109.9 kB | | semver | 7.6.3 | 95.82 kB | 95.82 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.1 | 51.46 kB | 51.46 kB | | int64-buffer | 0.1.10 | 49.18 kB | 49.18 kB | | shell-quote | 1.8.1 | 44.96 kB | 44.96 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.3.1 | 25.21 kB | 25.21 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | dc-polyfill | 0.1.4 | 23.1 kB | 23.1 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | path-to-regexp | 0.1.10 | 6.38 kB | 6.38 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

@pr-commenter
Copy link

pr-commenter bot commented Oct 23, 2024

Benchmarks

Benchmark execution time: 2024-10-29 13:41:37

Comparing candidate commit 3ef1ead in PR branch watson/DEBUG-2804/crude-snapshot-pruning with baseline commit 49d6c58 in branch master.

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

juan-fernandez
juan-fernandez previously approved these changes Oct 24, 2024
Base automatically changed from watson/DEBUG-2804/refactor-integration-tests to master October 28, 2024 09:20
@watson watson dismissed juan-fernandez’s stale review October 28, 2024 09:20

The base branch was changed.

@watson watson force-pushed the watson/DEBUG-2804/crude-snapshot-pruning branch from f0041a7 to 87e143e Compare October 28, 2024 09:34
juan-fernandez
juan-fernandez previously approved these changes Oct 28, 2024
shatzi
shatzi previously approved these changes Oct 28, 2024

if (Buffer.byteLength(json) > MAX_PAYLOAD_SIZE) {
// TODO: This is a very crude way to handle large payloads. Proper pruning will be implemented later (DEBUG-2624)
delete payload['debugger.snapshot']
Copy link

Choose a reason for hiding this comment

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

kinda harsh :D

remove 'debugger.snapshot' would cause probe-tracker to mark the probe as error: as the probe would trigger emitting start but there are not events marked with the probe-id and would default to error state.

I would suggest remove 'debugger.snapshot.captures' or replace it with notCaptureReasons: 'snapshot was too large' - at least give the user some feedback on why the don't see the snapshot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, good point. My intent was to only rmeove the captures part, but the JSON schema threw me for a loop since we're collecting a snapshot, the property is called snapshot and it's called "snapshot pruning" 🙄 I'll push a fix 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed 👍

The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
@watson watson dismissed stale reviews from shatzi and juan-fernandez via 1d824e5 October 29, 2024 13:21
@watson watson force-pushed the watson/DEBUG-2804/crude-snapshot-pruning branch from 87e143e to 1d824e5 Compare October 29, 2024 13:21
@watson watson requested a review from shatzi October 29, 2024 14:36
Copy link

@shatzi shatzi left a comment

Choose a reason for hiding this comment

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

very cool idea to make the tests robust to line changes using // BREAKPOINT

@watson watson merged commit 111c14a into master Oct 30, 2024
205 checks passed
@watson watson deleted the watson/DEBUG-2804/crude-snapshot-pruning branch October 30, 2024 07:04
rochdev pushed a commit that referenced this pull request Oct 31, 2024
The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
rochdev pushed a commit that referenced this pull request Oct 31, 2024
The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
rochdev pushed a commit that referenced this pull request Oct 31, 2024
The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
rochdev pushed a commit that referenced this pull request Nov 6, 2024
The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
rochdev pushed a commit that referenced this pull request Nov 6, 2024
The log track has a 1MB limit of the JSON payload. The client is not
notified if the payload is too large, but it is simply never indexed.

This is a very crude approach. In the future a more sophsticated
algorithm will be implemented that reduces the size of the snapshot
instead of removing it completely.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants