Add script version to worker trace events. #1445
Conversation
src/workerd/api/trace.h
Outdated
| @@ -100,6 +102,7 @@ class TraceItem final: public jsg::Object { | |||
| kj::Maybe<kj::String> dispatchNamespace; | |||
| jsg::Optional<kj::Array<kj::String>> scriptTags; | |||
| kj::String outcome; | |||
| kj::Maybe<kj::String> scriptVersion; | |||
There was a problem hiding this comment.
could you add a comment a two about Maybe here? What does it mean? Not every script has a version? It is empty first and changed later?
There was a problem hiding this comment.
It means not every script necessarily has a version. The same question could be asked of all the other Maybe fields here, so I think we should either add similar comments for all optional fields or none?
| @@ -39,6 +39,7 @@ struct Trace @0x8e8d911203762d34 { | |||
|
|
|||
| outcome @2 :EventOutcome; | |||
| scriptName @4 :Text; | |||
| scriptVersion @19 :Text; | |||
There was a problem hiding this comment.
Should this be scriptVersionId to be most specific?
There was a problem hiding this comment.
Yeah, agree.
- Update to
scriptVersionId
jp4a50
force-pushed
the
jphillips/trace-script-version
branch
from
6c1162f to
ec08613
Compare
| @@ -71,6 +71,11 @@ class TraceItem final: public jsg::Object { | |||
| kj::ArrayPtr<jsg::Ref<TraceException>> getExceptions(); | |||
| kj::ArrayPtr<jsg::Ref<TraceDiagnosticChannelEvent>> getDiagnosticChannelEvents(); | |||
| kj::Maybe<kj::StringPtr> getScriptName(); | |||
| // TODO(someday): we expose this as jsg::Optional for now, since that ensures that the property will | |||
There was a problem hiding this comment.
@jasnell I've gone for this approach to "hide" the scriptVersionId property for now when it's not explicitly set. I did briefly consider making it a *_PROTOTYPE_PROPERTY instead to further "hide" it as they don't show up when calling Object.keys() but that seems like it would be weirdly inconsistent with the other properties here. LMK what you think.
jp4a50
force-pushed
the
jphillips/trace-script-version
branch
from
ec08613 to
b7e14e6
Compare
|
Last push was rebase only. |
No description provided.