[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #2: Make async stuff work reliably
kezhenxu94 commented on pull request #2: URL: https://github.com/apache/skywalking-nodejs/pull/2#issuecomment-735480908 > I am seeing a `Request failed with status code 500`, is this an actual failure and not a test config thing? And if so how can I get the actual failing code to check? It's an actual failure because the actual trace data is different from the expected (in the `expected.data.yaml` file) There is the actual trace data https://github.com/apache/skywalking-nodejs/pull/2/checks?check_run_id=1470200800#step:5:11660 ๏ผremove the quotes, `+`, `\n`, etc.) you can compare it with the `expected.data.yaml` to see what is different This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #2: Make async stuff work reliably
kezhenxu94 commented on pull request #2: URL: https://github.com/apache/skywalking-nodejs/pull/2#issuecomment-735248247 > * Context.capture() and restore() will be made redundant / unnecessary by this? Yes I believe they're unnecessary after this PR, feel free to remove them altogether in this PR or in a following PR if you like. > * ContextManagere.withSpan() export to user? Like what I commented, `ContextManagere.withSpan()` seems to be not that useful but just for catching exceptions? I don't want to add another callback level in the users codes. > * Node 10 does not have AsyncLocalStorage, will need to provide custom substitute class for this (using async_hooks.createHook init and destroy to track parent-child relationships). Let's tackle Node10 a little bit later after this PR. "using async_hooks.createHook init and destroy to track parent-child relationships" this is my original thought before coming across `AsyncLocalStorage`, but be careful, we need to evaluate the memory usage and performance impact when tracking the parent-child relationship, (does `AsyncLocalStorage` do so behind the scene?) > * I had this question with the Python agent whether entry spans should always start with a new context but decided against it because maybe someone wants a previous local context for some debug or other reason to chain before the entry context? So I did the same here and disabled fresh context on Entry span but rather just let it use the normal inheritance chain, which should eventually give a new context unless the user specifically prevents it. ๐ > * Has problem as Python agent with background updater not firing if application exits very quickly, should not exit until all data sent, probably should hook 'beforeExit' event and flush segments, should do same in Python. I'd consider this as a bug (not sending segment when it exists very quickly). Let's create some issues to track those aforementioned that are not to be addressed in this PR ๐ This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #2: Make async stuff work reliably
kezhenxu94 commented on pull request #2: URL: https://github.com/apache/skywalking-nodejs/pull/2#issuecomment-735243085 > No not quite ready to merge, need to show cases where this works and previous one doesn't and want to export `ContextManager` for user specified spans. Hoping for later today or tomorrow latest. Take your time, feel free to ping me when it's ready, thanks in advance ๐ This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #2: Make async stuff work reliably
kezhenxu94 commented on pull request #2: URL: https://github.com/apache/skywalking-nodejs/pull/2#issuecomment-735242587 @tom-pytel is it ready to review/merge now? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [skywalking-nodejs] kezhenxu94 commented on pull request #2: Make async stuff work reliably
kezhenxu94 commented on pull request #2: URL: https://github.com/apache/skywalking-nodejs/pull/2#issuecomment-735231523 > Configuration error I think, I don't get the "no-shadowed-variable Shadowed name: ?" error locally when I build or lint. What IDE are you using? I'm using WebStorm, make sure to enable tslint and prettier, or run `npm run lint` in the command line, also please rebase on the master (I enable the listing on master but the PR seems to be stale This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
