From 7a68f76ebb83afd64c46a87d07e4d869c431392a Mon Sep 17 00:00:00 2001 From: Jon Phillips Date: Tue, 5 Dec 2023 20:50:21 +0000 Subject: [PATCH] Replace scriptVersionId with scriptVersion object in worker trace events. --- src/workerd/api/trace.c++ | 30 ++++++++++++++++++++++++--- src/workerd/api/trace.h | 21 ++++++++++++------- src/workerd/io/BUILD.bazel | 7 +++++++ src/workerd/io/script-version.capnp | 17 +++++++++++++++ src/workerd/io/trace.c++ | 19 +++++++++-------- src/workerd/io/trace.h | 8 +++---- src/workerd/io/worker-interface.capnp | 3 ++- 7 files changed, 81 insertions(+), 24 deletions(-) create mode 100644 src/workerd/io/script-version.capnp diff --git a/src/workerd/api/trace.c++ b/src/workerd/api/trace.c++ index afc9e1ba02b..335849f5017 100644 --- a/src/workerd/api/trace.c++ +++ b/src/workerd/api/trace.c++ @@ -80,6 +80,10 @@ kj::Array> getTraceDiagnosticChannelEvents }; } +kj::Maybe> getTraceScriptVersion(const Trace& trace) { + return trace.scriptVersion.map([](const auto& version) { return capnp::clone(*version); }); +} + double getTraceExceptionTimestamp(const Trace::Exception& ex) { if (isPredictableModeForTest()) { return 0; @@ -172,7 +176,7 @@ TraceItem::TraceItem(jsg::Lock& js, const Trace& trace) exceptions(getTraceExceptions(trace)), diagnosticChannelEvents(getTraceDiagnosticChannelEvents(js, trace)), scriptName(trace.scriptName.map([](auto& name) { return kj::str(name); })), - scriptVersionId(trace.scriptVersionId.map([](auto& id) { return kj::str(id); })), + scriptVersion(getTraceScriptVersion(trace)), dispatchNamespace(trace.dispatchNamespace.map([](auto& ns) { return kj::str(ns); })), scriptTags(getTraceScriptTags(trace)), outcome(getTraceOutcome(trace)), @@ -214,8 +218,8 @@ kj::Maybe TraceItem::getScriptName() { return scriptName.map([](auto& name) -> kj::StringPtr { return name; }); } -jsg::Optional TraceItem::getScriptVersionId() { - return scriptVersionId.map([](auto& scriptVersionId) -> kj::StringPtr { return scriptVersionId; }); +jsg::Optional TraceItem::getScriptVersion() { + return scriptVersion.map([](auto& version) { return ScriptVersion(*version); }); } jsg::Optional TraceItem::getDispatchNamespace() { @@ -416,6 +420,26 @@ jsg::JsValue TraceDiagnosticChannelEvent::getMessage(jsg::Lock& js) { double TraceDiagnosticChannelEvent::getTimestamp() { return timestamp; } +ScriptVersion::ScriptVersion(workerd::ScriptVersion::Reader version) + : id{[&]() -> kj::Maybe { + if (version.hasId()) { + return kj::str(version.getId()); + } + return kj::none; + }()}, + tag{[&]() -> kj::Maybe { + if (version.hasTag()) { + return kj::str(version.getTag()); + } + return kj::none; + }()}, + message{[&]() -> kj::Maybe { + if (version.hasMessage()) { + return kj::str(version.getMessage()); + } + return kj::none; + }()} {} + TraceItem::CustomEventInfo::CustomEventInfo(const Trace& trace, const Trace::CustomEventInfo& eventInfo) : eventInfo(eventInfo) {} diff --git a/src/workerd/api/trace.h b/src/workerd/api/trace.h index 144ee5c2725..eb5f31d16a1 100644 --- a/src/workerd/api/trace.h +++ b/src/workerd/api/trace.h @@ -45,6 +45,16 @@ class TailEvent final: public ExtendableEvent { } }; +struct ScriptVersion { + explicit ScriptVersion(workerd::ScriptVersion::Reader version); + + jsg::Optional id; + jsg::Optional tag; + jsg::Optional message; + + JSG_STRUCT(id, tag, message); +}; + class TraceItem final: public jsg::Object { public: class FetchEventInfo; @@ -71,11 +81,7 @@ class TraceItem final: public jsg::Object { kj::ArrayPtr> getExceptions(); kj::ArrayPtr> getDiagnosticChannelEvents(); kj::Maybe getScriptName(); - // TODO(someday): we expose this as jsg::Optional for now, since that ensures that the property will - // not show up when calling JSON.stringify() on the object if it has not been explicitly set. - // At some point, we may want to align the behaviour with getScriptName() which shows up as - // `null` when not explicitly set. - jsg::Optional getScriptVersionId(); + jsg::Optional getScriptVersion(); jsg::Optional getDispatchNamespace(); jsg::Optional> getScriptTags(); kj::StringPtr getOutcome(); @@ -90,7 +96,7 @@ class TraceItem final: public jsg::Object { JSG_LAZY_READONLY_INSTANCE_PROPERTY(exceptions, getExceptions); JSG_LAZY_READONLY_INSTANCE_PROPERTY(diagnosticsChannelEvents, getDiagnosticChannelEvents); JSG_LAZY_READONLY_INSTANCE_PROPERTY(scriptName, getScriptName); - JSG_LAZY_READONLY_INSTANCE_PROPERTY(scriptVersionId, getScriptVersionId); + JSG_LAZY_READONLY_INSTANCE_PROPERTY(scriptVersion, getScriptVersion); JSG_LAZY_READONLY_INSTANCE_PROPERTY(dispatchNamespace, getDispatchNamespace); JSG_LAZY_READONLY_INSTANCE_PROPERTY(scriptTags, getScriptTags); JSG_LAZY_READONLY_INSTANCE_PROPERTY(outcome, getOutcome); @@ -103,7 +109,7 @@ class TraceItem final: public jsg::Object { kj::Array> exceptions; kj::Array> diagnosticChannelEvents; kj::Maybe scriptName; - jsg::Optional scriptVersionId; + kj::Maybe> scriptVersion; kj::Maybe dispatchNamespace; jsg::Optional> scriptTags; kj::String outcome; @@ -433,6 +439,7 @@ class TraceCustomEventImpl final: public WorkerInterface::CustomEvent { }; #define EW_TRACE_ISOLATE_TYPES \ + api::ScriptVersion, \ api::TailEvent, \ api::TraceItem, \ api::TraceItem::AlarmEventInfo, \ diff --git a/src/workerd/io/BUILD.bazel b/src/workerd/io/BUILD.bazel index d231bbed027..a452183bf17 100644 --- a/src/workerd/io/BUILD.bazel +++ b/src/workerd/io/BUILD.bazel @@ -141,6 +141,7 @@ wd_cc_capnp_library( visibility = ["//visibility:public"], deps = [ ":outcome_capnp", + ":script_version_capnp", ":trimmed-supported-compatibility-date", "@capnp-cpp//src/capnp/compat:http-over-capnp_capnp", ], @@ -152,6 +153,12 @@ wd_cc_capnp_library( visibility = ["//visibility:public"], ) +wd_cc_capnp_library( + name = "script_version_capnp", + srcs = ["script-version.capnp"], + visibility = ["//visibility:public"], +) + [kj_test( src = f, deps = [ diff --git a/src/workerd/io/script-version.capnp b/src/workerd/io/script-version.capnp new file mode 100644 index 00000000000..422eaa5435f --- /dev/null +++ b/src/workerd/io/script-version.capnp @@ -0,0 +1,17 @@ +# Copyright (c) 2023 Cloudflare, Inc. +# Licensed under the Apache 2.0 license found in the LICENSE file or at: +# https://opensource.org/licenses/Apache-2.0 + +@0xd3b6bb739ff1b77a; + +using Cxx = import "/capnp/c++.capnp"; +$Cxx.namespace("workerd"); + +struct ScriptVersion { + id @0 :Text; + # An ID that should be used to uniquely identify this version. + tag @1 :Text; + # An optional tag to associate with this version. + message @2 :Text; + # An optional message that can be used to describe this version. +} diff --git a/src/workerd/io/trace.c++ b/src/workerd/io/trace.c++ index 85dd6dd4439..174522653c2 100644 --- a/src/workerd/io/trace.c++ +++ b/src/workerd/io/trace.c++ @@ -3,6 +3,7 @@ // https://opensource.org/licenses/Apache-2.0 #include +#include #include #include #include @@ -185,11 +186,11 @@ Trace::Exception::Exception(kj::Date timestamp, kj::String name, kj::String mess : timestamp(timestamp), name(kj::mv(name)), message(kj::mv(message)) {} Trace::Trace(kj::Maybe stableId, kj::Maybe scriptName, - kj::Maybe scriptVersionId, kj::Maybe dispatchNamespace, + kj::Maybe> scriptVersion, kj::Maybe dispatchNamespace, kj::Array scriptTags) : stableId(kj::mv(stableId)), scriptName(kj::mv(scriptName)), - scriptVersionId(kj::mv(scriptVersionId)), + scriptVersion(kj::mv(scriptVersion)), dispatchNamespace(kj::mv(dispatchNamespace)), scriptTags(kj::mv(scriptTags)) {} Trace::Trace(rpc::Trace::Reader reader) { @@ -219,8 +220,8 @@ void Trace::copyTo(rpc::Trace::Builder builder) { KJ_IF_SOME(name, scriptName) { builder.setScriptName(name); } - KJ_IF_SOME(id, scriptVersionId) { - builder.setScriptVersionId(id); + KJ_IF_SOME(version, scriptVersion) { + builder.setScriptVersion(*version); } KJ_IF_SOME(ns, dispatchNamespace) { builder.setDispatchNamespace(ns); @@ -303,14 +304,14 @@ void Trace::mergeFrom(rpc::Trace::Reader reader, PipelineLogLevel pipelineLogLev // mergeFrom() is called both when deserializing traces from a sandboxed // worker and when deserializing traces sent to a sandboxed trace worker. In // the former case, the trace's scriptName (and other fields like - // scriptVersionId) are already set and the deserialized value is missing, so + // scriptVersion) are already set and the deserialized value is missing, so // we need to be careful not to overwrite the set value. if (reader.hasScriptName()) { scriptName = kj::str(reader.getScriptName()); } - if (reader.hasScriptVersionId()) { - scriptVersionId = kj::str(reader.getScriptVersionId()); + if (reader.hasScriptVersion()) { + scriptVersion = capnp::clone(reader.getScriptVersion()); } if (reader.hasDispatchNamespace()) { @@ -448,9 +449,9 @@ kj::Promise>> PipelineTracer::onComplete() { kj::Own PipelineTracer::makeWorkerTracer( PipelineLogLevel pipelineLogLevel, kj::Maybe stableId, - kj::Maybe scriptName, kj::Maybe scriptVersionId, + kj::Maybe scriptName, kj::Maybe> scriptVersion, kj::Maybe dispatchNamespace, kj::Array scriptTags) { - auto trace = kj::refcounted(kj::mv(stableId), kj::mv(scriptName), kj::mv(scriptVersionId), + auto trace = kj::refcounted(kj::mv(stableId), kj::mv(scriptName), kj::mv(scriptVersion), kj::mv(dispatchNamespace), kj::mv(scriptTags)); traces.add(kj::addRef(*trace)); return kj::refcounted(kj::addRef(*this), kj::mv(trace), pipelineLogLevel); diff --git a/src/workerd/io/trace.h b/src/workerd/io/trace.h index 51660f8e0c9..1681f54b52f 100644 --- a/src/workerd/io/trace.h +++ b/src/workerd/io/trace.h @@ -50,8 +50,8 @@ enum class PipelineLogLevel { class Trace final : public kj::Refcounted { public: explicit Trace(kj::Maybe stableId, kj::Maybe scriptName, - kj::Maybe scriptVersionId, kj::Maybe dispatchNamespace, - kj::Array scriptTags); + kj::Maybe> scriptVersion, + kj::Maybe dispatchNamespace, kj::Array scriptTags); Trace(rpc::Trace::Reader reader); ~Trace() noexcept(false); KJ_DISALLOW_COPY_AND_MOVE(Trace); @@ -233,7 +233,7 @@ class Trace final : public kj::Refcounted { // trace, if any. kj::Maybe scriptName; - kj::Maybe scriptVersionId; + kj::Maybe> scriptVersion; kj::Maybe dispatchNamespace; kj::Array scriptTags; @@ -295,7 +295,7 @@ class PipelineTracer final : public kj::Refcounted { kj::Own makeWorkerTracer(PipelineLogLevel pipelineLogLevel, kj::Maybe stableId, kj::Maybe scriptName, - kj::Maybe scriptVersionId, + kj::Maybe> scriptVersion, kj::Maybe dispatchNamespace, kj::Array scriptTags); // Makes a tracer for a worker stage. diff --git a/src/workerd/io/worker-interface.capnp b/src/workerd/io/worker-interface.capnp index 9df7f2c784a..a039c9931dc 100644 --- a/src/workerd/io/worker-interface.capnp +++ b/src/workerd/io/worker-interface.capnp @@ -12,6 +12,7 @@ $Cxx.namespace("workerd::rpc"); using import "/capnp/compat/http-over-capnp.capnp".HttpMethod; using import "/capnp/compat/http-over-capnp.capnp".HttpService; using import "/workerd/io/outcome.capnp".EventOutcome; +using import "/workerd/io/script-version.capnp".ScriptVersion; struct Trace @0x8e8d911203762d34 { logs @0 :List(Log); @@ -39,7 +40,7 @@ struct Trace @0x8e8d911203762d34 { outcome @2 :EventOutcome; scriptName @4 :Text; - scriptVersionId @19 :Text; + scriptVersion @19 :ScriptVersion; eventTimestampNs @5 :Int64;