Skip to content

Safer shimming#4585

Merged
bengl merged 18 commits intomasterfrom
bengl/safe-shimming
Aug 15, 2024
Merged

Safer shimming#4585
bengl merged 18 commits intomasterfrom
bengl/safe-shimming

Conversation

@bengl
Copy link
Copy Markdown
Collaborator

@bengl bengl commented Aug 8, 2024

What does this PR do?

Adds a layer of try/catch around instrumentation code, and makes a best-effort attempt to attribute errors to instrumentation (i.e. rather than the instrumented code). In those cases, it handles the errors transparently. This is all only enabled in SSI.

Additional Notes

Caveats about code that isn't covered here are mentioned in the code comments.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 8, 2024

Overall package size

Self size: 6.95 MB
Deduped: 58.17 MB
No deduping: 58.45 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/native-appsec | 8.0.1 | 15.59 MB | 15.6 MB | | @datadog/native-iast-taint-tracking | 3.1.0 | 12.27 MB | 12.28 MB | | @datadog/pprof | 5.3.0 | 9.85 MB | 10.22 MB | | protobufjs | 7.2.5 | 2.77 MB | 5.16 MB | | @datadog/native-iast-rewriter | 2.4.1 | 2.14 MB | 2.23 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.8.1 | 71.67 kB | 785.15 kB | | msgpack-lite | 0.1.26 | 201.16 kB | 281.59 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 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 | | lru-cache | 7.14.0 | 74.95 kB | 74.95 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 | | 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 | | path-to-regexp | 0.1.7 | 6.78 kB | 6.78 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 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
Copy Markdown

pr-commenter Bot commented Aug 8, 2024

Benchmarks

Benchmark execution time: 2024-08-15 16:50:13

Comparing candidate commit 12a8da8 in PR branch bengl/safe-shimming with baseline commit 4df8c4d in branch master.

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

@bengl bengl force-pushed the bengl/safe-shimming branch from 19538fa to 5403726 Compare August 8, 2024 10:33
@bengl bengl marked this pull request as ready for review August 8, 2024 10:33
@bengl bengl requested a review from a team as a code owner August 8, 2024 10:33
@simon-id
Copy link
Copy Markdown
Member

simon-id commented Aug 8, 2024

I don't have time to review today but what kind of error would it be able to handle ?

@bengl
Copy link
Copy Markdown
Collaborator Author

bengl commented Aug 8, 2024

@simon-id this is specifically covering errors thrown in instrumentation code on the publisher side of the diagnostics channel.

Qard
Qard previously approved these changes Aug 8, 2024
Copy link
Copy Markdown
Contributor

@Qard Qard left a comment

Choose a reason for hiding this comment

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

LGTM, just one suggestion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment to explain why all this is here but commented out.

@bengl bengl marked this pull request as draft August 8, 2024 21:02
@bengl
Copy link
Copy Markdown
Collaborator Author

bengl commented Aug 8, 2024

@Qard I converted this back to draft because I'm concerned that shimmer.wrap() may have been broken by this approach, when SSI is enabled. Until I verify that's all good, I'd prefer to leave this in draft mode.

@bengl bengl force-pushed the bengl/safe-shimming branch from 835f2d8 to fb72908 Compare August 12, 2024 15:45
@bengl bengl marked this pull request as ready for review August 12, 2024 18:48
@bengl bengl requested review from a team as code owners August 12, 2024 18:48
@bengl bengl requested a review from anmarchenko August 12, 2024 18:48
@bengl bengl enabled auto-merge (squash) August 12, 2024 18:48
@bengl bengl force-pushed the bengl/safe-shimming branch from 991f083 to ab544d8 Compare August 12, 2024 20:26
Qard
Qard previously approved these changes Aug 13, 2024
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 14, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.46%. Comparing base (156d510) to head (12a8da8).
Report is 2 commits behind head on master.

Files Patch % Lines
...kages/datadog-instrumentations/src/mocha/common.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4585      +/-   ##
==========================================
- Coverage   86.52%   84.46%   -2.06%     
==========================================
  Files          20      284     +264     
  Lines        1150    12309   +11159     
  Branches       33       33              
==========================================
+ Hits          995    10397    +9402     
- Misses        155     1912    +1757     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Qard
Qard previously approved these changes Aug 15, 2024
@bengl bengl force-pushed the bengl/safe-shimming branch from 540c85b to 919c2d0 Compare August 15, 2024 15:26
@bengl bengl merged commit 69aedbc into master Aug 15, 2024
@bengl bengl deleted the bengl/safe-shimming branch August 15, 2024 17:45
bengl added a commit that referenced this pull request Aug 29, 2024
* add a safety layer to shimmed functions

* tests

* make it work with recursion

* wrap only once per function instead of per invocation

* lint

* hoist error handler, to be once per function instead of per invocation

* no need to delete holder values since the variable isn't re-used anymore

* cleanup

* safeWrap non-methods

* ensure wrapFunction is used whenever wrap was used with 2 args

* fix package guadrails test
@bengl bengl mentioned this pull request Aug 29, 2024
bengl added a commit that referenced this pull request Aug 29, 2024
* add a safety layer to shimmed functions

* tests

* make it work with recursion

* wrap only once per function instead of per invocation

* lint

* hoist error handler, to be once per function instead of per invocation

* no need to delete holder values since the variable isn't re-used anymore

* cleanup

* safeWrap non-methods

* ensure wrapFunction is used whenever wrap was used with 2 args

* fix package guadrails test
@bengl bengl mentioned this pull request Aug 29, 2024
bengl added a commit that referenced this pull request Aug 30, 2024
* add a safety layer to shimmed functions

* tests

* make it work with recursion

* wrap only once per function instead of per invocation

* lint

* hoist error handler, to be once per function instead of per invocation

* no need to delete holder values since the variable isn't re-used anymore

* cleanup

* safeWrap non-methods

* ensure wrapFunction is used whenever wrap was used with 2 args

* fix package guadrails test
bengl added a commit that referenced this pull request Aug 30, 2024
* add a safety layer to shimmed functions

* tests

* make it work with recursion

* wrap only once per function instead of per invocation

* lint

* hoist error handler, to be once per function instead of per invocation

* no need to delete holder values since the variable isn't re-used anymore

* cleanup

* safeWrap non-methods

* ensure wrapFunction is used whenever wrap was used with 2 args

* fix package guadrails test
kodiakhq Bot referenced this pull request in X-oss-byte/Nextjs Aug 31, 2024
This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [dd-trace](https://togithub.com/DataDog/dd-trace-js) | [`5.21.0` -> `5.22.0`](https://renovatebot.com/diffs/npm/dd-trace/5.10.0/5.22.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/dd-trace/5.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/dd-trace/5.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/dd-trace/5.10.0/5.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/dd-trace/5.10.0/5.22.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>DataDog/dd-trace-js (dd-trace)</summary>

### [`v5.22.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.22.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.21.0...v5.22.0)

##### Fixes

-   \[[`ab80d70`](https://togithub.com/DataDog/dd-trace-js/commit/ab80d703c1)] - fix(lambda): gate timeout spans and add missing clear (jordan gonzález) [https://github.com/DataDog/dd-trace-js/pull/4446](https://togithub.com/DataDog/dd-trace-js/pull/4446)
-   \[[`1f2914f`](https://togithub.com/DataDog/dd-trace-js/commit/1f2914f9fc)] - chore(w3c): avoid setting \_dd.parent_id to 16 zeros (Munir Abdinur)[https://github.com/DataDog/dd-trace-js/pull/4576](https://togithub.com/DataDog/dd-trace-js/pull/4576)6
-   \[[`683df27`](https://togithub.com/DataDog/dd-trace-js/commit/683df27a6d)] - fix(openai): tracing occurs in correct async context (Sam Brenner) [https://github.com/DataDog/dd-trace-js/pull/4583](https://togithub.com/DataDog/dd-trace-js/pull/4583)
-   \[[`b19b382`](https://togithub.com/DataDog/dd-trace-js/commit/b19b382f0a)] - Rename rasp metrics (Igor Unanua) [https://github.com/DataDog/dd-trace-js/pull/4635](https://togithub.com/DataDog/dd-trace-js/pull/4635)
-   \[[`f4ecee9`](https://togithub.com/DataDog/dd-trace-js/commit/f4ecee9633)] - Fix AppSec manual blocking parameters (Ugaitz Urien) [https://github.com/DataDog/dd-trace-js/pull/4606](https://togithub.com/DataDog/dd-trace-js/pull/4606)

##### Features

-   \[[`bc2d6a1`](https://togithub.com/DataDog/dd-trace-js/commit/bc2d6a1473)] - Bump `@datadog/native-appsec` to `8.1.1` (Carles Capell) [https://github.com/DataDog/dd-trace-js/pull/4630](https://togithub.com/DataDog/dd-trace-js/pull/4630)
-   \[[`1a2aaed`](https://togithub.com/DataDog/dd-trace-js/commit/1a2aaedaf0)] - \[test visibility] Add configuration flags to auto test retries (Juan Antonio Fernández de Alba)[https://github.com/DataDog/dd-trace-js/pull/4609](https://togithub.com/DataDog/dd-trace-js/pull/4609)9

##### Improvements

-   \[[`e568fe3`](https://togithub.com/DataDog/dd-trace-js/commit/e568fe3e8f)] - PROF-10330: Emit SSI information in the profile system info (Attila Szegedi)
-   \[[`a04ba3f`](https://togithub.com/DataDog/dd-trace-js/commit/a04ba3f9e7)] - hapi: migrate from AsyncResource to TracingChannel, see [#&#8203;4597](https://togithub.com/DataDog/dd-trace-js/issues/4597) (Thomas Hunter II) [https://github.com/DataDog/dd-trace-js/pull/4622](https://togithub.com/DataDog/dd-trace-js/pull/4622)
-   \[[`69aedbc`](https://togithub.com/DataDog/dd-trace-js/commit/69aedbc892)] - Safer shimming (Bryan English) [https://github.com/DataDog/dd-trace-js/pull/4585](https://togithub.com/DataDog/dd-trace-js/pull/4585)

### [`v5.21.0`](https://togithub.com/DataDog/dd-trace-js/compare/v5.20.0...725f16142baf7724f537a6848dc49e1b10d889bb)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.20.0...v5.21.0)

### [`v5.20.0`](https://togithub.com/DataDog/dd-trace-js/compare/v5.19.0...v5.20.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.19.0...v5.20.0)

### [`v5.19.0`](https://togithub.com/DataDog/dd-trace-js/compare/v5.18.0...37d3876ded2e8a2fce14520bf85dc6920e777c31)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.18.0...v5.19.0)

### [`v5.18.0`](https://togithub.com/DataDog/dd-trace-js/compare/v5.17.0...01464c7c14154ef75e6ce90b283d2e85b55cbb5c)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.17.0...v5.18.0)

### [`v5.17.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.17.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.16.0...v5.17.0)

##### Improvements

-   **core:** RecordException api now supports adding exceptions as span events ([#&#8203;4386](https://togithub.com/DataDog/dd-trace-js/issues/4386))
-   **appsec:** Allows AppSec to replace status code, headers, and body when an attack is detected ([#&#8203;3837](https://togithub.com/DataDog/dd-trace-js/issues/3837))

##### Features

-   **core:** Add support for span events ([#&#8203;4036](https://togithub.com/DataDog/dd-trace-js/issues/4036) )
-   **core:** Add automatic instrumentation support for Undici ([#&#8203;4293](https://togithub.com/DataDog/dd-trace-js/issues/4293))

### [`v5.16.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.16.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.15.0...v5.16.0)

##### Improvements

-   **graphql:** fix graphql.resolve span durations ([#&#8203;4387](https://togithub.com/DataDog/dd-trace-js/issues/4387))
-   **otel:** remaps http status tag to meet ddog convention for otel([#&#8203;4384](https://togithub.com/DataDog/dd-trace-js/issues/4384))

##### Bug Fixes

-   **ci:** test OCI system-tests scenarios on every commit ([#&#8203;4371](https://togithub.com/DataDog/dd-trace-js/issues/4371))
-   **otel:** pin otel api version in integration test ([#&#8203;4388](https://togithub.com/DataDog/dd-trace-js/issues/4388))
-   **core:** send original config value to telemetry ([#&#8203;4378](https://togithub.com/DataDog/dd-trace-js/issues/4378))
-   **core:** fix RC support for sampling rules patch ([#&#8203;4381](https://togithub.com/DataDog/dd-trace-js/issues/4381))

### [`v5.15.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.15.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.14.1...v5.15.0)

#### Improvements

-   Remove outdated polyfills [#&#8203;4009](https://togithub.com/DataDog/dd-trace-js/pull/4009)
-   Update native-appsec module to 8.0.1 [#&#8203;4347](https://togithub.com/DataDog/dd-trace-js/pull/4347)
-   profiler supports activation through single-step instrumentation [#&#8203;4375](https://togithub.com/DataDog/dd-trace-js/pull/4375)

#### Features

-   \[ci-visibility] Support mocha parallel mode [#&#8203;4314](https://togithub.com/DataDog/dd-trace-js/pull/4314)

#### Bug fixes

-   \[ci-visibility] Fix EFD with jest and jsdom [#&#8203;4323](https://togithub.com/DataDog/dd-trace-js/pull/4323)
-   \[ci-visibility] Fix selenium when run outside of a supported test framework [#&#8203;4330](https://togithub.com/DataDog/dd-trace-js/pull/4330)

### [`v5.14.1`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.14.1)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.14.0...v5.14.1)

##### Features

-   debug warnings when init after instrumented packages ([#&#8203;4307](https://togithub.com/DataDog/dd-trace-js/issues/4307))

### [`v5.14.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.14.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.13.0...v5.14.0)

##### Improvements

-   **core:** Add child_process plugin to typings ([#&#8203;4306](https://togithub.com/DataDog/dd-trace-js/issues/4306)), thanks to [@&#8203;ikonst](https://togithub.com/ikonst) for the original PR
-   **asm:** Add support for meta_struct property in the spans for v0.4 agent api ([#&#8203;4287](https://togithub.com/DataDog/dd-trace-js/issues/4287))
-   **serverless:** Enable Serverless Mini Agent for Azure Functions on All Plans ([#&#8203;4304](https://togithub.com/DataDog/dd-trace-js/issues/4304))
-   **core:** stop clobbering manually-installed tracer with Single Step Instrumentation ([#&#8203;4300](https://togithub.com/DataDog/dd-trace-js/issues/4300))
-   **core:** define an explicit version range of support for the [@&#8203;opentelemetry/api](https://togithub.com/opentelemetry/api) ([#&#8203;4318](https://togithub.com/DataDog/dd-trace-js/issues/4318))

##### Features

-   **profiling:** Add profiler support for node 22 ([#&#8203;4312](https://togithub.com/DataDog/dd-trace-js/issues/4312))

### [`v5.13.0`](https://togithub.com/DataDog/dd-trace-js/compare/v5.12.0...0325725d8a4195ad980fcbe9535dc3f54f3c591c)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.12.0...v5.13.0)

### [`v5.12.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.12.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.11.0...v5.12.0)

##### Features

-   [Add Support for OpenAI v4](https://togithub.com/DataDog/dd-trace-js/pull/4232)

##### Improvements

-   [Do not require appsec modules when disabling appsec if they have not been required before](https://togithub.com/DataDog/dd-trace-js/pull/4244)

##### Bug Fixes

-   [fix fetch error when request has already been sent](https://togithub.com/DataDog/dd-trace-js/pull/4258). Fixes [#&#8203;4259](https://togithub.com/DataDog/dd-trace-js/issues/4259)
-   [Prevent object key tainting](https://togithub.com/DataDog/dd-trace-js/pull/4251)

### [`v5.11.0`](https://togithub.com/DataDog/dd-trace-js/releases/tag/v5.11.0)

[Compare Source](https://togithub.com/DataDog/dd-trace-js/compare/v5.10.0...v5.11.0)

##### Features

-   \[profiling] [PROF-9250: Enable timeline and CPU profiling by default](https://togithub.com/DataDog/dd-trace-js/pull/4149)
-   \[iast] [Hardcoded passwords detection and update hardcoded secret rules](https://togithub.com/DataDog/dd-trace-js/pull/4066)
-   \[ci-visibility] [\[ci-visibility\] Add selenium support](https://togithub.com/DataDog/dd-trace-js/pull/4241)
-   \[open telemetry] [implement OTEL env var support for dd-trace-js](https://togithub.com/DataDog/dd-trace-js/pull/4248)
-   \[profiling] [Support ipv6 in profiler](https://togithub.com/DataDog/dd-trace-js/pull/4124). Thanks [@&#8203;benasher44](https://togithub.com/benasher44) for the PR!

##### Improvements

-   \[dogstatsd] [Added histogram method to dogstatsd](https://togithub.com/DataDog/dd-trace-js/pull/4178). Thanks [@&#8203;bin-umar](https://togithub.com/bin-umar) for the PR!

##### Bug Fixes

-   \[mongoose] [Fixed attempting to wrap undefined mongoose.Promise](https://togithub.com/DataDog/dd-trace-js/pull/4165). Thanks [@&#8203;nathan-knight](https://togithub.com/nathan-knight) for the PR!
-   \[mysql2] [Avoid run sequelize plugin test with non compatible mysql2](https://togithub.com/DataDog/dd-trace-js/pull/4229)
-   \[ci-visibility] [\[ci-visibility\] Fix window access when using cy.origin (multi origin) in Cypress](https://togithub.com/DataDog/dd-trace-js/pull/4228)
-   \[dogstatsd] [Add dogstatsd to noop proxy](https://togithub.com/DataDog/dd-trace-js/pull/4225)
-   \[ci-visibility] [\[ci-visibility\] Fix ITR with multi project setup in jest](https://togithub.com/DataDog/dd-trace-js/pull/4249)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/). View the [repository job log](https://developer.mend.io/github/X-oss-byte/Nextjs).
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