add rewriter support for generator functions#7472
Conversation
Overall package sizeSelf size: 4.69 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.6 | 81.92 kB | 816.75 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7472 +/- ##
==========================================
- Coverage 80.34% 80.33% -0.01%
==========================================
Files 731 734 +3
Lines 31093 31583 +490
==========================================
+ Hits 24981 25373 +392
- Misses 6112 6210 +98
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bengl
left a comment
There was a problem hiding this comment.
Good start. Can't wait to see the corresponding PR against Orchestrion-JS!
| self: this, | ||
| moduleVersion: "1.0.0" | ||
| }; | ||
| const gen = ${channelVariable}.traceSync(__apm$traced, ctx); |
There was a problem hiding this comment.
I was going to suggest that this should be ${traceMethod}, but then I remembered that async generators return synchronously, despite being called async. 😆
There was a problem hiding this comment.
Yeah I actually did the same thing and wrote it wrong at first lol.
|
|
||
| if (callee) { | ||
| const expression = parse(` | ||
| Reflect.get(Object.getPrototypeOf(this.constructor.prototype), '${method}').call(this) |
There was a problem hiding this comment.
You may as well be consistent and use Reflect.getPrototypeOf here.
Also, can you leave a comment explaining why Reflect.get is necessary?
There was a problem hiding this comment.
Alright, I see you've added the comment, it it makes sense, but it doesn't explain why Reflect.get in particular is necessary.
There was a problem hiding this comment.
Wasn't really needed in this case so I just removed it.
wconti27
left a comment
There was a problem hiding this comment.
Thank you for this change @rochdev . This will really help us in instrumenting LLM/AI agent packages! Gonna leave the review up to our local node experts, but just wanted to express how helpful these Orchestrion changes are to us (IDM)!
|
@bengl Any other concerns with the PR? |
| const wrap = it => { | ||
| const { next: itNext, return: itReturn, throw: itThrow } = it; | ||
|
|
||
| it.next = (...args) => ${traceNext}(itNext, ctx, it, ...args); |
There was a problem hiding this comment.
Nit: I usually prefer iter as an instance of an iterator, but that's not a blocker.
There was a problem hiding this comment.
I need to fix wrapSuper so I'll do this at the same time.
There was a problem hiding this comment.
Fixed wrapSuper and renamed the variable.
This comment has been minimized.
This comment has been minimized.
packages/datadog-instrumentations/src/helpers/rewriter/transforms.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Crystal Luc-Magloire <[email protected]>
What does this PR do?
Add rewriter support for generator functions.
Motivation
They can't be patched using the current available functionality.
Additional Notes