Nextjs fix #110
Nextjs fix #110
Conversation
Copying files causes duplicate in `require.cache` which cause the main files to be executed twice if the observed application required the main file again -- eg: https://github.com/vercel/next.js/blob/7e5cb510c4155572d0b251db34845d34fd078480/packages/next/lib/get-project-dir.ts#L3 In addition, this new version of the abomination is less invasive and does not monkey-patch `module`.
There was a problem hiding this comment.
Awesome! I can confirm I'm able to launch a Next server via the agent launcher now.
Though, I'm still having trouble getting an HTTP response from the Next server. They monkey patch the Response.write method before returning the response which raises an exception because write is marked as a read only property. It looks like this is due to us setting it as non-writable here: https://github.com/applandinc/appmap-agent-js/blob/main/components/patch/default/index.mjs#L33-L39
It seems to me like the flags we are setting on the property should be inherited from the current descriptor instead of hard coded. What do you think?
Somewhat related to this, I'm not getting a route for _appmap/record. I didn't dig much into this.
I'm also seeing another issue where I can't attach a JavaScript debugger without crashing on launch. It doesn't look like it's related to this change so maybe it's a separate issue. It's something to do with unexpected (?) asynchronous operations: https://github.com/applandinc/appmap-agent-js/blob/main/components/hook-group/node/index.mjs#L57
Removing the assertions for id in group and similar allows the server to launch. Maybe these should just be warnings instead of panics? Generally I think it's better to potentially drop some data and log a warning instead of panicking. If this is unrelated, we can open a separate issue to track this.
Nice pinpoint of the root cause!
Well by monkey patching we are already screwing up the transparency of the analysis. For instance,
I'm not sure what you mean. You have trouble with remote recording? The request is forwarded to the observed application? This could be related. To reproduce the failure, you simply try to perform remote recording on the sample application from above?
The async_hooks API is very state heavy. Because I'm not entirely sure of when the hooks are triggered I rely on a fair amount of assertions. Most of these assertions are implicitly used during the post-processing of the trace to re-order event causally rather than chronologically. So I have to be careful when modifying these assertions to avoid causing hard-to-find problems downstream. Maybe crashing is too strong. But I do need to be notified when an assertion breaks so that I can adapt the post-processing of the trace. I feel that simple warnings will be ignored. What can be done? Anyway, this means that there is a case that I did not foresee. But as you said, this is a separate issue. |
Right, remote recording did not appear to be functioning. The request appeared as if it wasn't being intercepted, returning a 404 instead of the expected 200. To test, I was simply using curl to retrieve the recording status of the agent: $ curl localhost:3000/_appmap/record |
|
The above also seemed to only happen on Mac which makes me wonder if it was just an issue with my environment. I'll need to circle back on that. The remote recording endpoint does appear working on Linux. During some further testing I found that we're not able to hook the functions in pages compiled by next. They use webpack under the hood and the pages they emit end up looking something like: (() => {
var exports = {};
exports.id = "pages/index";
exports.ids = ["pages/index"];
exports.modules = {
"./pages/index.js": ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => {
eval("<WEBPACKED JS>");
}
}
})();Actually hooking the page logic would require we also transform the source code string passed to That's sort of where things stand at the moment. I'm compiling a debug binary of |
|
Hi @dustin! Sorry I let this in the backburner for too long. I'm in holidays without a laptop. You seem to have stumble upon an issue linked to code transformation. In my experience fixing those are hard but I had the most success by reducing the code to be instrumented as much as possible and then reverse engineer the location of the bug. Sorry to not be able to help more. I will be back the 15th of August. I might be able to find a laptop if it is critical to solve the issue before that. Btw there still #108 that is waiting for a review. I don't think it will this issue but who knows? |
FWIW, I'm going to take a look at #108. |
@dividedmind Thanks! I released it and answered your comments. Feel free to circle back on them. |
I can confirm that the remote recording works fine in
The dreaded
Any progress on that front before I jump into it? |
Actually, I'm not sure what you mean by: hooking the functions in pages compiled by next. Are these functions executed in the server or in the client? If it is executed on the client it is beyond our reach (for the moment). If it is executed on the server, I don't understand the concept of: function in pages. How important is it to record them? This confusion is probably due to my lack of knowledge of |
Defining monkey patches as constant properties was too restrictive. The
idea was to be notified when the observed application would insert its
own conflicting monkey patch. But the application could behave well and
call our own monkey patch. This the case for instance for nextjs which
has somewhere:
```js
function compression(e,o,c){
var u=false;
var m;
var d=[];
var x;
var v=o.end;
var f=o.on;
var b=o.write;
o.flush=function flush(){
if(x){x.flush()}
};
o.write=function write(e,a){
if(u){return false}
if(!this._header){this._implicitHeader()}
return x?x.write(toBuffer(e,a)):b.call(this,e,a)
};
```
Fixed by cb392b3 |
No, I ended up switching priorities instead of digging deeper into this.
With |
|
@dustinbyrne Thanks for the clarification :) How important is it to record these functions? I remember when I started the agent we discussed with @kgilpin about instrumenting code provided to |
|
This is a case where the server-side logic is compiled by Webpack and emitted out as a string passed to eval. The source map is inlined inside the source string so everything should map back to code present in the file system. I'm guessing web frameworks like Nuxt work in a similar fashion. Also worth mentioning that it works differently for production builds. The source code is written directly to the file instead of being eval'd into the runtime. |
|
It's worth mentioning that I've only just discovered the production build. Maybe we already have some support for production-only builds? |
`escodegen` is largely unmaintained at this point. `astring` is a good alternative. We also considered `@babel/generator` but it is more heavy weight. We only need support for estree compliant input, no source mapping and no minification.
There is no reason to keep these two hooks inside a single component. All the other hooks have their dedicated component. Also remove deadcode for transformSourceDefault.
`hook-esm` now provide functions that can directly be provided to the hook esm module api of node.
The configuration format exposed to the user was `config` and becomes `external-configuration`. And the compiled configuration was `configuration` and becomes `internal-configuration`.
Also, change if-else structure for readability.
Call expressions that are considered as eval call expressions will have their first argument instrumented. This enables to record functions provided as strings to eval calls. This functionality is disabled by default.
By passing the old value to a callback instead of returning it, we prevent polluting the caller's scope. It looks nicer.
There was a problem hiding this comment.
Thanks! Please consider splitting the ordering comments across the relevant files and expanding on them a bit more before you merge. Also note I haven't actually tried out the code, mine is only a reading review, so if you're not confident it works as expected, do some more testing, ask someone else (unless @dustinbyrne is already on it), and/or provide easy instructions for me to try it out. You might also consider cleaning up the commit history for a rebase merge, although it looks daunting so I won't blame you if you do a merge commit.
Before, the events caused by the user's request handler was not associated with the request/response event pair.
|
@dividedmind thanks for the valuable input! As soon as the CI passes I will merge :) |
|
🎉 This PR is included in version 11.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #109