Skip to content

fix(openai): tracing occurs in correct async context#4583

Merged
sabrenner merged 9 commits intomasterfrom
sabrenner/openai-tracing-fix
Aug 13, 2024
Merged

fix(openai): tracing occurs in correct async context#4583
sabrenner merged 9 commits intomasterfrom
sabrenner/openai-tracing-fix

Conversation

@sabrenner
Copy link
Copy Markdown
Collaborator

@sabrenner sabrenner commented Aug 7, 2024

What does this PR do?

Ensures tracing and parentage is tracked appropriately across different async contexts by migrating the OpenAI plugin to use tracing channel/appropriate internal APIs.

Motivation

Previously, something like:

await tracer.trace('outer', async () => {
  await openai.chat.completions.create(...)
  tracer.trace('child', () => {})
})

Would have child as a child span of the openai.request span, and not the outer span. By migrating to tracing channel and using the internal APIs for later versions, this parentage/context is maintained. This is validated with a regression test, for both streamed and non-streamed cases.

Plugin Checklist

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 7, 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 7, 2024

Benchmarks

Benchmark execution time: 2024-08-12 19:17:43

Comparing candidate commit bd2b48f in PR branch sabrenner/openai-tracing-fix with baseline commit ed9b0b3 in branch master.

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.19%. Comparing base (f82c6d5) to head (04adfbb).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4583      +/-   ##
==========================================
+ Coverage   63.02%   69.19%   +6.16%     
==========================================
  Files         254        1     -253     
  Lines       11095      198   -10897     
  Branches       33       33              
==========================================
- Hits         6993      137    -6856     
+ Misses       4102       61    -4041     

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

@sabrenner sabrenner marked this pull request as ready for review August 12, 2024 14:22
@sabrenner sabrenner requested review from a team as code owners August 12, 2024 14:22
@rochdev
Copy link
Copy Markdown
Member

rochdev commented Aug 12, 2024

By properly wrapping the traced functions in a shared async resource, this parentage/context is maintained.

Using AsyncResource alters the async context for all code in the process. Could we instead switch the openai integration over to TracingChannel to avoid unintended side-effects of altering the async stack?

@sabrenner
Copy link
Copy Markdown
Collaborator Author

Could we instead switch the openai integration over to TracingChannel to avoid unintended side-effects of altering the async stack?

For sure! I think this should be pretty doable for the patching of older versions of OpenAI (<=3), but I'm wondering if it would be a bit harder for the newer patching because of how we currently trace streamed responses (based on my understanding of TracingChannel). Happy to chat offline with more details, but if we have an instance of a more complex migration of an integration to TracingChannel, that might help me out.

@rochdev
Copy link
Copy Markdown
Member

rochdev commented Aug 12, 2024

if we have an instance of a more complex migration of an integration to TracingChannel, that might help me out.

There are grpc and http that both use the lower-level API since they are also stream-based.

@sabrenner
Copy link
Copy Markdown
Collaborator Author

@rochdev thanks! I think I've implemented this correctly, the test I added is still passing for both streamed and non-streamed cases, and i think it's following established patterns for other TracingChannel migrations for other plugins.

@sabrenner sabrenner merged commit 683df27 into master Aug 13, 2024
@sabrenner sabrenner deleted the sabrenner/openai-tracing-fix branch August 13, 2024 17:13
bengl pushed a commit that referenced this pull request Aug 29, 2024
* wrap in appropriate async resource

* test

* trigger ci

* remove .only

* lint

* slim tests

* v3 patching uses tracing channel directly

* add v4 patching using tracing channel internal api

* fixes and cleanup
@bengl bengl mentioned this pull request Aug 29, 2024
bengl pushed a commit that referenced this pull request Aug 29, 2024
* wrap in appropriate async resource

* test

* trigger ci

* remove .only

* lint

* slim tests

* v3 patching uses tracing channel directly

* add v4 patching using tracing channel internal api

* fixes and cleanup
@bengl bengl mentioned this pull request Aug 29, 2024
bengl pushed a commit that referenced this pull request Aug 30, 2024
* wrap in appropriate async resource

* test

* trigger ci

* remove .only

* lint

* slim tests

* v3 patching uses tracing channel directly

* add v4 patching using tracing channel internal api

* fixes and cleanup
bengl pushed a commit that referenced this pull request Aug 30, 2024
* wrap in appropriate async resource

* test

* trigger ci

* remove .only

* lint

* slim tests

* v3 patching uses tracing channel directly

* add v4 patching using tracing channel internal api

* fixes and cleanup
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