Replace scriptVersionId with scriptVersion object in worker trace events #1476
Conversation
jp4a50
force-pushed
the
jphillips/script-version
branch
2 times, most recently
from
e59a8f3 to
b9f5ecc
Compare
src/workerd/api/trace.h
Outdated
| @@ -45,6 +45,17 @@ class TailEvent final: public ExtendableEvent { | |||
| } | |||
| }; | |||
|
|
|||
| struct ScriptVersion { | |||
| public: | |||
| @@ -103,7 +110,7 @@ class TraceItem final: public jsg::Object { | |||
| kj::Array<jsg::Ref<TraceException>> exceptions; | |||
| kj::Array<jsg::Ref<TraceDiagnosticChannelEvent>> diagnosticChannelEvents; | |||
| kj::Maybe<kj::String> scriptName; | |||
| jsg::Optional<kj::String> scriptVersionId; | |||
| kj::Maybe<kj::Own<workerd::ScriptVersion::Reader>> scriptVersion; | |||
There was a problem hiding this comment.
It would probably be better here to store kj::Maybe<ScriptVersion> rather than the kj::Own<...>` but consider that non-blocking.
There was a problem hiding this comment.
I initially did that but then realized that I needed to copy the ScriptVersion to implement getScriptVersion, so it seemed easier to just retain the capnp object and pass it to the constructor each time rather than additionally having logic to copy a ScriptVersion.
| @@ -71,11 +82,7 @@ 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.
Having looked at the other fields in here I don't actually think using jsg::Optional (such that unpopulated fields manifest as undefined in JS) is actually any more inconsistent than using jsg/kj::Maybe so I don't think it's actually worth changing it in the future, especially since it will lead to a subtle change in behaviour of an existing field.
| @@ -0,0 +1,17 @@ | |||
| # Copyright (c) 2023 Cloudflare, Inc. | |||
There was a problem hiding this comment.
Any reason this is hanging out on it's own, and not in worker-interface.capnp?
There was a problem hiding this comment.
So it can be pulled in as a isolated dependency for other (internal) uses.
jp4a50
force-pushed
the
jphillips/script-version
branch
from
b9f5ecc to
762f543
Compare
|
Last push is rebase only. |
jp4a50
force-pushed
the
jphillips/script-version
branch
from
762f543 to
7a68f76
Compare
Follow up to #1445.
We have since decided that we want to expose additional fields (provisionally
tagandmessage) associated with the script version. As such, we replace the stringscriptVersionIdfield with ascriptVersionobject in the JS API.The API is not used yet so we don't need to worry about backwards compatibility here.