From b651a109188bc6200be2a5b93a587880a2867e6b Mon Sep 17 00:00:00 2001
From: Joe Lee <jlee@cloudflare.com>
Date: Thu, 31 Aug 2023 12:12:39 -0700
Subject: [PATCH 1/2] Modify actor channel setup to let getActor tracing spans
 appear nested under "actor_subrequest" spans

This does have the side effect of skipping the lazy actor stub initialization
if an intervening step of the fetcher setup fails (e.g. when the worker is over
request limits), but I think this is unlikely to have user-visible behavior,
and may in fact be desirable, in that it avoids unnecessary getActor() calls.
---
 src/workerd/api/actor.c++   | 24 +++++++++++-------------
 src/workerd/io/io-context.h | 11 ++++++-----
 2 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/src/workerd/api/actor.c++ b/src/workerd/api/actor.c++
index 3410539dbf5..fec6ff2e240 100644
--- a/src/workerd/api/actor.c++
+++ b/src/workerd/api/actor.c++
@@ -23,18 +23,17 @@ public:
   kj::Own<WorkerInterface> newSingleUseClient(kj::Maybe<kj::String> cfStr) override {
     auto& context = IoContext::current();
 
-    // Lazily initialize actorChannel
-    if (actorChannel == nullptr) {
-      auto& context = IoContext::current();
-      actorChannel = context.getColoLocalActorChannel(channelId, actorId);
-    }
-
     return context.getMetrics().wrapActorSubrequestClient(context.getSubrequest(
         [&](SpanBuilder& span, IoChannelFactory& ioChannelFactory) {
       if (span.isObserved()) {
         span.setTag("actor_id"_kjc, kj::str(actorId));
       }
 
+      // Lazily initialize actorChannel
+      if (actorChannel == nullptr) {
+        actorChannel = context.getColoLocalActorChannel(channelId, actorId, span);
+      }
+
       return KJ_REQUIRE_NONNULL(actorChannel)->startRequest({
         .cfBlobJson = kj::mv(cfStr),
         .parentSpan = span
@@ -67,19 +66,18 @@ public:
   kj::Own<WorkerInterface> newSingleUseClient(kj::Maybe<kj::String> cfStr) override {
     auto& context = IoContext::current();
 
-    // Lazily initialize actorChannel
-    if (actorChannel == nullptr) {
-      auto& context = IoContext::current();
-      actorChannel = context.getGlobalActorChannel(channelId, id->getInner(), kj::mv(locationHint),
-          mode);
-    }
-
     return context.getMetrics().wrapActorSubrequestClient(context.getSubrequest(
         [&](SpanBuilder& span, IoChannelFactory& ioChannelFactory) {
       if (span.isObserved()) {
         span.setTag("actor_id"_kjc, id->toString());
       }
 
+      // Lazily initialize actorChannel
+      if (actorChannel == nullptr) {
+        actorChannel = context.getGlobalActorChannel(channelId, id->getInner(), kj::mv(locationHint),
+            mode, span);
+      }
+
       return KJ_REQUIRE_NONNULL(actorChannel)->startRequest({
         .cfBlobJson = kj::mv(cfStr),
         .parentSpan = span
diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h
index 5021d62d0c0..3db14c5188d 100644
--- a/src/workerd/io/io-context.h
+++ b/src/workerd/io/io-context.h
@@ -835,13 +835,14 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
   }
 
   kj::Own<IoChannelFactory::ActorChannel> getGlobalActorChannel(
-        uint channel, const ActorIdFactory::ActorId& id, kj::Maybe<kj::String> locationHint,
-        ActorGetMode mode) {
+      uint channel, const ActorIdFactory::ActorId& id, kj::Maybe<kj::String> locationHint,
+      ActorGetMode mode, SpanParent parentSpan) {
     return getIoChannelFactory().getGlobalActor(channel, id, kj::mv(locationHint), mode,
-        getCurrentTraceSpan());
+        kj::mv(parentSpan));
   }
-  kj::Own<IoChannelFactory::ActorChannel> getColoLocalActorChannel(uint channel, kj::StringPtr id) {
-    return getIoChannelFactory().getColoLocalActor(channel, id, getCurrentTraceSpan());
+  kj::Own<IoChannelFactory::ActorChannel> getColoLocalActorChannel(uint channel, kj::StringPtr id,
+      SpanParent parentSpan) {
+    return getIoChannelFactory().getColoLocalActor(channel, id, kj::mv(parentSpan));
   }
 
   // Get an HttpClient to use for Cache API subrequests.

From eb2450598a21fdbb98e83084846d797812431250 Mon Sep 17 00:00:00 2001
From: Joe Lee <jlee@cloudflare.com>
Date: Thu, 31 Aug 2023 13:33:22 -0700
Subject: [PATCH 2/2] Clarify contract of IoContext::getSubrequest() lambda
 parameter

---
 src/workerd/io/io-context.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h
index 3db14c5188d..6732f3edffa 100644
--- a/src/workerd/io/io-context.h
+++ b/src/workerd/io/io-context.h
@@ -786,7 +786,8 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
       kj::FunctionParam<kj::Own<WorkerInterface>(SpanBuilder&, IoChannelFactory&)> func,
       SubrequestOptions options);
 
-  // If creating a new subrequest is permitted, calls the given factory function to create one.
+  // If creating a new subrequest is permitted, calls the given factory function synchronously to
+  // create one.
   kj::Own<WorkerInterface> getSubrequest(
       kj::FunctionParam<kj::Own<WorkerInterface>(SpanBuilder&, IoChannelFactory&)> func,
       SubrequestOptions options);