Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace scriptVersionId with scriptVersion object in worker trace events #1476

Merged
merged 1 commit into from
Dec 11, 2023
Merged

Replace scriptVersionId with scriptVersion object in worker trace events #1476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/workerd/api/trace.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ kj::Array<jsg::Ref<TraceDiagnosticChannelEvent>> getTraceDiagnosticChannelEvents
};
}

kj::Maybe<kj::Own<workerd::ScriptVersion::Reader>> 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;
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -214,8 +218,8 @@ kj::Maybe<kj::StringPtr> TraceItem::getScriptName() {
return scriptName.map([](auto& name) -> kj::StringPtr { return name; });
}

jsg::Optional<kj::StringPtr> TraceItem::getScriptVersionId() {
return scriptVersionId.map([](auto& scriptVersionId) -> kj::StringPtr { return scriptVersionId; });
jsg::Optional<ScriptVersion> TraceItem::getScriptVersion() {
return scriptVersion.map([](auto& version) { return ScriptVersion(*version); });
}

jsg::Optional<kj::StringPtr> TraceItem::getDispatchNamespace() {
Expand Down Expand Up @@ -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<kj::String> {
if (version.hasId()) {
return kj::str(version.getId());
}
return kj::none;
}()},
tag{[&]() -> kj::Maybe<kj::String> {
if (version.hasTag()) {
return kj::str(version.getTag());
}
return kj::none;
}()},
message{[&]() -> kj::Maybe<kj::String> {
if (version.hasMessage()) {
return kj::str(version.getMessage());
}
return kj::none;
}()} {}

TraceItem::CustomEventInfo::CustomEventInfo(const Trace& trace,
const Trace::CustomEventInfo& eventInfo)
: eventInfo(eventInfo) {}
Expand Down
21 changes: 14 additions & 7 deletions src/workerd/api/trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,16 @@ class TailEvent final: public ExtendableEvent {
}
};

struct ScriptVersion {
explicit ScriptVersion(workerd::ScriptVersion::Reader version);

jsg::Optional<kj::String> id;
jsg::Optional<kj::String> tag;
jsg::Optional<kj::String> message;

JSG_STRUCT(id, tag, message);
};

class TraceItem final: public jsg::Object {
public:
class FetchEventInfo;
Expand All @@ -71,11 +81,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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this still valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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<kj::StringPtr> getScriptVersionId();
jsg::Optional<ScriptVersion> getScriptVersion();
jsg::Optional<kj::StringPtr> getDispatchNamespace();
jsg::Optional<kj::Array<kj::StringPtr>> getScriptTags();
kj::StringPtr getOutcome();
Expand All @@ -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);
Expand All @@ -103,7 +109,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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would probably be better here to store kj::Maybe<ScriptVersion> rather than the kj::Own<...>` but consider that non-blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kj::Maybe<kj::String> dispatchNamespace;
jsg::Optional<kj::Array<kj::String>> scriptTags;
kj::String outcome;
Expand Down Expand Up @@ -433,6 +439,7 @@ class TraceCustomEventImpl final: public WorkerInterface::CustomEvent {
};

#define EW_TRACE_ISOLATE_TYPES \
api::ScriptVersion, \
api::TailEvent, \
api::TraceItem, \
api::TraceItem::AlarmEventInfo, \
Expand Down
7 changes: 7 additions & 0 deletions src/workerd/io/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
Expand All @@ -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 = [
Expand Down
17 changes: 17 additions & 0 deletions src/workerd/io/script-version.capnp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Copyright (c) 2023 Cloudflare, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is hanging out on it's own, and not in worker-interface.capnp?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it can be pulled in as a isolated dependency for other (internal) uses.

# 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.
}
19 changes: 10 additions & 9 deletions src/workerd/io/trace.c++
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// https://opensource.org/licenses/Apache-2.0

#include <workerd/io/trace.h>
#include <capnp/message.h>
#include <capnp/schema.h>
#include <kj/compat/http.h>
#include <kj/debug.h>
Expand Down Expand Up @@ -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<kj::String> stableId, kj::Maybe<kj::String> scriptName,
kj::Maybe<kj::String> scriptVersionId, kj::Maybe<kj::String> dispatchNamespace,
kj::Maybe<kj::Own<ScriptVersion::Reader>> scriptVersion, kj::Maybe<kj::String> dispatchNamespace,
kj::Array<kj::String> 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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -448,9 +449,9 @@ kj::Promise<kj::Array<kj::Own<Trace>>> PipelineTracer::onComplete() {

kj::Own<WorkerTracer> PipelineTracer::makeWorkerTracer(
PipelineLogLevel pipelineLogLevel, kj::Maybe<kj::String> stableId,
kj::Maybe<kj::String> scriptName, kj::Maybe<kj::String> scriptVersionId,
kj::Maybe<kj::String> scriptName, kj::Maybe<kj::Own<ScriptVersion::Reader>> scriptVersion,
kj::Maybe<kj::String> dispatchNamespace, kj::Array<kj::String> scriptTags) {
auto trace = kj::refcounted<Trace>(kj::mv(stableId), kj::mv(scriptName), kj::mv(scriptVersionId),
auto trace = kj::refcounted<Trace>(kj::mv(stableId), kj::mv(scriptName), kj::mv(scriptVersion),
kj::mv(dispatchNamespace), kj::mv(scriptTags));
traces.add(kj::addRef(*trace));
return kj::refcounted<WorkerTracer>(kj::addRef(*this), kj::mv(trace), pipelineLogLevel);
Expand Down
8 changes: 4 additions & 4 deletions src/workerd/io/trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ enum class PipelineLogLevel {
class Trace final : public kj::Refcounted {
public:
explicit Trace(kj::Maybe<kj::String> stableId, kj::Maybe<kj::String> scriptName,
kj::Maybe<kj::String> scriptVersionId, kj::Maybe<kj::String> dispatchNamespace,
kj::Array<kj::String> scriptTags);
kj::Maybe<kj::Own<ScriptVersion::Reader>> scriptVersion,
kj::Maybe<kj::String> dispatchNamespace, kj::Array<kj::String> scriptTags);
Trace(rpc::Trace::Reader reader);
~Trace() noexcept(false);
KJ_DISALLOW_COPY_AND_MOVE(Trace);
Expand Down Expand Up @@ -233,7 +233,7 @@ class Trace final : public kj::Refcounted {
// trace, if any.

kj::Maybe<kj::String> scriptName;
kj::Maybe<kj::String> scriptVersionId;
kj::Maybe<kj::Own<ScriptVersion::Reader>> scriptVersion;
kj::Maybe<kj::String> dispatchNamespace;
kj::Array<kj::String> scriptTags;

Expand Down Expand Up @@ -295,7 +295,7 @@ class PipelineTracer final : public kj::Refcounted {
kj::Own<WorkerTracer> makeWorkerTracer(PipelineLogLevel pipelineLogLevel,
kj::Maybe<kj::String> stableId,
kj::Maybe<kj::String> scriptName,
kj::Maybe<kj::String> scriptVersionId,
kj::Maybe<kj::Own<ScriptVersion::Reader>> scriptVersion,
kj::Maybe<kj::String> dispatchNamespace,
kj::Array<kj::String> scriptTags);
// Makes a tracer for a worker stage.
Expand Down
3 changes: 2 additions & 1 deletion src/workerd/io/worker-interface.capnp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -39,7 +40,7 @@ struct Trace @0x8e8d911203762d34 {

outcome @2 :EventOutcome;
scriptName @4 :Text;
scriptVersionId @19 :Text;
scriptVersion @19 :ScriptVersion;

eventTimestampNs @5 :Int64;

Expand Down