⚗️ Add instrumentConstructor utility#4714
Conversation
Bundles Sizes Evolution
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c445e3c783
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 8d89c48 | Docs | Datadog PR Page | Give us feedback! |
bdibon
force-pushed
the
boris.dibon/ws-1-instrument-method-constructor
branch
from
c445e3c to
e2bdea2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2bdea2f62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b52da5151
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f0b7a0fda
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| return replaceWithInstrumentation(target, name, original, (isStopped) => { | ||
| const instrumentation = function (this: TARGET): ConstructorInstanceOf<TARGET[NAME]> { |
There was a problem hiding this comment.
💬 suggestion: this is actually not TARGET here but since we don't use it, we could just remove it
| const instrumentation = function (this: TARGET): ConstructorInstanceOf<TARGET[NAME]> { | |
| const instrumentation = function (): ConstructorInstanceOf<TARGET[NAME]> { |
There was a problem hiding this comment.
For the types, I've decided to rename TARGET to CONTAINER which is more accurate in our context (instrumenting global constructors).
I am using this when I replicate the native behavior for constructor called without new (i.e runtime throws an exception). I probably could have used undefined instead, but I preferred to be defensive and not to temper with what the caller of the instrumentation was doing. In this context, this is unknown.
bdibon
force-pushed
the
boris.dibon/ws-1-instrument-method-constructor
branch
from
0f0b7a0 to
751858d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 751858d195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 201cc19f39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Co-authored-by: Cursor <cursoragent@cursor.com>
…d method after stop
bdibon
force-pushed
the
boris.dibon/ws-1-instrument-method-constructor
branch
from
201cc19 to
8d89c48
Compare
| } | ||
|
|
||
| interface InstrumentationCallbacks<RESULT> { | ||
| onPostCall: (callback: (result: RESULT) => void) => void |
There was a problem hiding this comment.
After instrumentation, instance.constructor === window.WebSocket returns false because prototype.constructor still points to the original. I can't think of real-world code that checks this, so it's probably fine, but wanted to confirm it's a known acceptable limitation. What do you think?
There was a problem hiding this comment.
You are right for instances that have been constructed before instrumentation, I have documented it here.
bdibon
changed the title
bdibon
changed the title
3 participants
Motivation
The WebSocket instrumentation added higher in this stack needs to wrap the
WebSocketconstructor, butinstrumentMethodonly supported instrumenting regular methods.Changes
instrumentMethod: handlenew.target, call the original viaReflect.construct, and preserve the originalprototypesoinstanceofchecks against the instrumented global keep working.MethodParameters/MethodReturnTypetype helpers that resolve the parameters/return type from either a call or a construct signature.Test instructions
yarn test:unit --spec packages/core/src/tools/instrumentMethod.spec.tsChecklist