call setEventInfo() for custom events#175
Merged
dom96 merged 2 commits intocloudflare:mainfrom Nov 17, 2022
Merged
Conversation
Custom events do not currently get a timestamp assigned, instead being logged with an unix epoch value. Create a custom event info, empty for now, so that we can correctly set the timestamp.
edevil
force-pushed
the
custom_event_info
branch
from
cf72d78 to
c09f717
Compare
Contributor
Author
dom96
approved these changes
Nov 17, 2022
Contributor
dom96
left a comment
dom96
left a comment
There was a problem hiding this comment.
Looks good to me. With the caveat that I don't have a lot of experience with this code path, but even if this doesn't work the change seems safe.
a-robinson
reviewed
Nov 17, 2022
Member
a-robinson
left a comment
a-robinson
left a comment
There was a problem hiding this comment.
Yeah I don't know if this is what we'll want when we start wanting to provide info specific to a given custom event type, but this looks fine for getting the timestamp populated.
kentonv
reviewed
Nov 17, 2022
|
|
||
| auto& context = incomingRequest->getContext(); | ||
| KJ_IF_MAYBE(t, incomingRequest->getWorkerTracer()) { | ||
| t->setEventInfo(context.now(), Trace::CustomEventInfo()); |
Member
There was a problem hiding this comment.
I think this code belongs inside the custom event callback, not here. That way the event can potentially decide what type of info to add. Additionally, you would have avoided the need for #177 as you could have this code execute after incomingRequest->delivered().
Contributor
Author
There was a problem hiding this comment.
Makes sense. I'll look into it tomorrow.
penalosa
pushed a commit
to penalosa/workerd
that referenced
this pull request
Nov 18, 2022
* Improve implementation of DOMException (cloudflare#159) * Implements API for connecting to TCP servers in workers * Adds TCP sockets sample implementing a Gopher proxy. * call setEventInfo() for custom events (cloudflare#175) * call setEventInfo() for custom events Custom events do not currently get a timestamp assigned, instead being logged with an unix epoch value. Create a custom event info, empty for now, so that we can correctly set the timestamp. * Update src/workerd/io/trace.h Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com> * fix context.now() when handling a trace The TraceCustomEventImpl is a custom event and when handling it the incomingRequests list is empty, causing getCurrentIncomingRequest() to fail. A new version of IoContext::now() is introduced for when we already have an incoming request available. * Add missing `locked` property to `ReadableStream` define * Add `CfHostMetadata` type parameter to `Request` override Allows the `HostMetadata` type parameter on `IncomingRequestCfProperties` to be specified with `Request` Co-authored-by: James M Snell <jsnell@cloudflare.com> Co-authored-by: Dominik Picheta <dominik@cloudflare.com> Co-authored-by: André Cruz <acruz@cloudflare.com> Co-authored-by: Dominik Picheta <dominikpicheta@googlemail.com>
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.
Custom events do not currently get a timestamp assigned, instead being logged with an unix epoch value. Create a custom event info, empty for now, so that we can correctly set the timestamp.