EW-9282 EW-9451 Fix missing onset event for JSRpc edge cases, add JSRpc regression test#4881
Merged
EW-9282 EW-9451 Fix missing onset event for JSRpc edge cases, add JSRpc regression test#4881
Conversation
177d671 to
f55505a
Compare
kentonv
reviewed
Aug 25, 2025
kentonv
reviewed
Aug 25, 2025
mar-cf
approved these changes
Aug 25, 2025
34462c5 to
bfbaadd
Compare
kentonv
previously requested changes
Aug 25, 2025
Member
kentonv
left a comment
There was a problem hiding this comment.
As a person who frequently modifies js-rpc-test, I do not want to be on the hook for updating this tracing test every time. Please find a different approach.
If we invoke a customEvent, we should generally report an onset event to the tracer right away. Under the STW model, we must do so if there's any chance for events like spans/logs being generated from JS (this wasn't the case with LTW so far). For JSRPC, we would only create an onset event after we were able to successfully get a function or property to call, but we may invoke JS code before that (e.g. if a JS getter method produced a log). Accordingly, set the onset as soon as we know the function name to report with it to avoid getting errors based on other events being sent before the onset. Note that this does result in traces being generated in some error cases (e.g. when a function lookup fails), but this is arguably the right approach, those JsRpcSessionCustomEventImpl invocations shouldn't just disappear.
bfbaadd to
74a4403
Compare
Contributor
Author
Updated the PR to only add a simple regression test in a new file. With this change, js-rpc-test.js is no longer coupled to the tail worker test. |
Calling a JSRPC getter method would result in "expected onsetSeen; Tail stream onset was not reported" STW errors.
74a4403 to
2f1622c
Compare
My issue is addressed. (I haven't read the rest of the code.)
fhanau
added a commit
that referenced
this pull request
Aug 29, 2025
…o late In #4881, we fixed the missing/delayed Onset issue for some cases, but to handle logs etc. being created in constructors we need to set the event before invoking JS at all. This requires some slight code duplication so we can access the method name in time.
fhanau
added a commit
that referenced
this pull request
Aug 29, 2025
…o late In #4881, we fixed the missing/delayed Onset issue for some cases, but to handle logs etc. being created in constructors we need to set the event before invoking JS at all. This requires some slight code duplication so we can access the method name in time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EW-9282 EW-9451 Fix missing onset event for JSRpc edge cases
If we invoke a customEvent, we should generally report an onset event to the tracer right away. Under the STW model, we must do so if there's any chance for events like spans/logs being generated from JS (this wasn't the case with LTW so far).
For JSRPC, we would only create an onset event after we were able to successfully get a function or property to call, but we may invoke JS code before that (e.g. if a JS getter method produced a log). Accordingly, set the onset as soon as we know the function name to report with it to avoid getting errors based on other events being sent before the onset.
Note that this does result in traces being generated in some error cases (e.g. when a function lookup fails), but this is arguably the right approach, those JsRpcSessionCustomEventImpl invocations shouldn't just disappear.
EW-9282 EW-9451 Add missing JSRPC onset regression test
Calling a JSRPC getter method would result in "expected onsetSeen; Tail stream onset was not reported" STW errors.