[nfc] Clean up r2GetClient(), removing getHttpClientWithSpans()

fhanau · fhanau · commit 76fd8bee904a · 2025-12-10T19:10:32.000-05:00
It turns out that the getHttpClient variant with trace context parameter
is easier to use.
diff --git a/src/workerd/api/r2-bucket.c++ b/src/workerd/api/r2-bucket.c++
@@ -25,17 +25,21 @@
 namespace workerd::api::public_beta {
 kj::Own<kj::HttpClient> r2GetClient(
     IoContext& context, uint subrequestChannel, R2UserTracing user) {
-  kj::Vector<Span::Tag> tags;
-  tags.add("rpc.service"_kjc, kj::ConstString("r2"_kjc));
-  tags.add(user.method.key, kj::ConstString(kj::str(user.method.value)));
+  auto traceSpan = context.makeTraceSpan(user.op);
+  auto userSpan = context.makeUserTraceSpan(user.op);
+  TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan));
+  traceContext.userSpan.setTag("rpc.service"_kjc, "r2"_kjc);
+  traceContext.userSpan.setTag(user.method.key, user.method.value);
   KJ_IF_SOME(b, user.bucket) {
-    tags.add("cloudflare.r2.bucket"_kjc, kj::ConstString(kj::str(b)));
+    traceContext.userSpan.setTag("cloudflare.r2.bucket"_kjc, b);
   }
   KJ_IF_SOME(tag, user.extraTag) {
-    tags.add(tag.key, kj::ConstString(kj::str(tag.value)));
+    traceContext.userSpan.setTag(tag.key, tag.value);
   }
 
-  return context.getHttpClientWithSpans(subrequestChannel, true, kj::none, user.op, kj::mv(tags));
+  // TODO(o11y): Attach trace context to awaitIo call to match operation lifetime better?
+  return context.getHttpClient(subrequestChannel, true, kj::none, traceContext)
+      .attach(kj::mv(traceContext));
 }
 
 static bool isWholeNumber(double x) {
diff --git a/src/workerd/io/io-context.c++ b/src/workerd/io/io-context.c++
@@ -971,26 +971,6 @@ kj::Own<WorkerInterface> IoContext::getSubrequestChannel(
       });
 }
 
-kj::Own<WorkerInterface> IoContext::getSubrequestChannelWithSpans(uint channel,
-    bool isInHouse,
-    kj::Maybe<kj::String> cfBlobJson,
-    kj::ConstString operationName,
-    kj::Vector<Span::Tag> tags) {
-  return getSubrequest(
-      [&](TraceContext& tracing, IoChannelFactory& channelFactory) {
-    for (Span::Tag& tag: tags) {
-      tracing.userSpan.setTag(kj::mv(tag.key), kj::mv(tag.value));
-    }
-    return getSubrequestChannelImpl(
-        channel, isInHouse, kj::mv(cfBlobJson), tracing, channelFactory);
-  },
-      SubrequestOptions{
-        .inHouse = isInHouse,
-        .wrapMetrics = !isInHouse,
-        .operationName = kj::mv(operationName),
-      });
-}
-
 kj::Own<WorkerInterface> IoContext::getSubrequestChannelNoChecks(uint channel,
     bool isInHouse,
     kj::Maybe<kj::String> cfBlobJson,
@@ -1029,15 +1009,6 @@ kj::Own<kj::HttpClient> IoContext::getHttpClient(
       getSubrequestChannel(channel, isInHouse, kj::mv(cfBlobJson), kj::mv(operationName)));
 }
 
-kj::Own<kj::HttpClient> IoContext::getHttpClientWithSpans(uint channel,
-    bool isInHouse,
-    kj::Maybe<kj::String> cfBlobJson,
-    kj::ConstString operationName,
-    kj::Vector<Span::Tag> tags) {
-  return asHttpClient(getSubrequestChannelWithSpans(
-      channel, isInHouse, kj::mv(cfBlobJson), kj::mv(operationName), kj::mv(tags)));
-}
-
 kj::Own<kj::HttpClient> IoContext::getHttpClient(
     uint channel, bool isInHouse, kj::Maybe<kj::String> cfBlobJson, TraceContext& traceContext) {
   return asHttpClient(getSubrequestChannel(channel, isInHouse, kj::mv(cfBlobJson), traceContext));
diff --git a/src/workerd/io/io-context.h b/src/workerd/io/io-context.h
@@ -855,12 +855,6 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
   kj::Own<WorkerInterface> getSubrequestChannel(
       uint channel, bool isInHouse, kj::Maybe<kj::String> cfBlobJson, TraceContext& traceContext);
 
-  kj::Own<WorkerInterface> getSubrequestChannelWithSpans(uint channel,
-      bool isInHouse,
-      kj::Maybe<kj::String> cfBlobJson,
-      kj::ConstString operationName,
-      kj::Vector<Span::Tag> tags);
-
   // Like getSubrequestChannel() but doesn't enforce limits. Use for trusted paths only.
   kj::Own<WorkerInterface> getSubrequestChannelNoChecks(uint channel,
       bool isInHouse,
@@ -877,13 +871,6 @@ class IoContext final: public kj::Refcounted, private kj::TaskSet::ErrorHandler
   kj::Own<kj::HttpClient> getHttpClient(
       uint channel, bool isInHouse, kj::Maybe<kj::String> cfBlobJson, TraceContext& traceContext);
 
-  // As above, but with list of span tags to add, analogous to getSubrequestChannelWithSpans().
-  kj::Own<kj::HttpClient> getHttpClientWithSpans(uint channel,
-      bool isInHouse,
-      kj::Maybe<kj::String> cfBlobJson,
-      kj::ConstString operationName,
-      kj::Vector<Span::Tag> tags);
-
   // Convenience methods that call getSubrequest*() and adapt the returned WorkerInterface objects
   // to HttpClient.
   kj::Own<kj::HttpClient> getHttpClientNoChecks(uint channel,
