Async Context Manager #183
Conversation
Populates span parent through most async usages that are important, allowing linking them to user interactions or custom parents made by the developer
| <button type="button" id="btn1">Button</button> | ||
|
|
||
| <script> | ||
| // Based on react/scheduler's implementation of nextTick |
There was a problem hiding this comment.
Right, but also a small test using actual React would be good. Also, we could use a rough initial documentation for the use cases it covers.
There was a problem hiding this comment.
There is an issue in jira to have framework tests
| <button type="button" id="btn1">Button</button> | ||
|
|
||
| <script> | ||
| // Based on vue's implementation of nextTick |
There was a problem hiding this comment.
Again, a tiny test case based on Vue.
| <button type="button" id="btn1">Button</button> | ||
|
|
||
| <script> | ||
| btn1.addEventListener('click', function () { |
There was a problem hiding this comment.
Can we merge all Promise context management tests into one? For example have one button per scenario. Then in a test you can execute clicks one by one - it will run faster.
| btn1.addEventListener('click', function () { | ||
| setTimeout(function () { | ||
| SplunkRum.provider.getTracer('default').startSpan('context-child').end(); | ||
| }, 1); |
There was a problem hiding this comment.
Can we. add a second button, with longer timeout? IIRC you exclude timeouts longer than 34 ms.
| // eslint-disable-next-line @typescript-eslint/no-this-alias | ||
| const manager = this; | ||
|
|
||
| wrapNatively(XMLHttpRequest.prototype, 'addEventListener', original => |
There was a problem hiding this comment.
but doesn't this belong in the XHR instrumentation?
There was a problem hiding this comment.
Would rather keep everything related to async trace connecting in one place (also made it way easier to config)
src/SplunkContextManager.ts
Outdated
| let wrapped = wrappedListeners.get(args[1] as EventListener); | ||
|
|
||
| if (!wrapped) { | ||
| wrapped = manager.bind(args[1] as EventListener, manager.active()); |
There was a problem hiding this comment.
Maybe it's intentional but I want to make sure, should we take the context which was active when .addEventListener() was called or the one active when XHR was created?
There was a problem hiding this comment.
Taking parent context as of registration is intentional, based on these thought processes in node side:
open-telemetry/opentelemetry-js#1479
open-telemetry/opentelemetry-js#2037
Firefox possibly runs timeouts (macrotask) before message event listeners (should be microtask)? At least seems like it in a few test runs
Populates span parent through most async usages that are important, allowing linking them to user interactions or custom parents made by the developer
Adds custom context manager that keeps parent through common async operations (eg. setTimeout, setImmediate (IE), Promise.then, ...). Patched targets are based on what's used in react (v17, newer) and vue (v2, v3), but should pretty much cover most of the things users do.
APMI-1716
Using todolist example (react)
Current version:
Separate traceid for /items which happens after rerender caused by processing response of /login
With these changes:
Both requests are in same traceid