Skip to content

Commit

Permalink
Merge pull request #2850 from cloudflare/dlapid/waitUntilTasks_trace
Browse files Browse the repository at this point in the history
Fix waitUntilTasks not empty when trace worker is over RPC
  • Loading branch information
danlapid authored Oct 7, 2024
2 parents c162362 + 51e3ab9 commit 0d58827
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 8 deletions.
11 changes: 5 additions & 6 deletions src/workerd/api/trace.c++
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ kj::Promise<void> sendTracesToExportedHandler(kj::Own<IoContext::IncomingRequest
jsg::AsyncContextFrame::StorageScope traceScope = context.makeAsyncTraceScope(lock);

auto handler = lock.getExportedHandler(entrypointName, context.getActor());
lock.getGlobalScope().sendTraces(traces, lock, handler);
return lock.getGlobalScope().sendTraces(traces, lock, handler);
});
} catch (kj::Exception e) {
// TODO(someday): We only report sendTraces() as failed for metrics/logging if the initial
Expand Down Expand Up @@ -654,11 +654,10 @@ auto TraceCustomEventImpl::sendRpc(capnp::HttpOverCapnpFactory& httpOverCapnpFac
traces[i]->copyTo(out[i]);
}

waitUntilTasks.add(req.send().ignoreResult());

// As long as we sent it, we consider the result to be okay.
co_return Result{
.outcome = workerd::EventOutcome::OK,
auto resp = co_await req.send();
auto respResult = resp.getResult();
co_return WorkerInterface::CustomEvent::Result{
.outcome = respResult.getOutcome(),
};
}

Expand Down
6 changes: 5 additions & 1 deletion src/workerd/io/worker-interface.capnp
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ struct Trace @0x8e8d911203762d34 {
# Indicates that the trace was truncated due to reaching the maximum size limit.
}

struct SendTracesRun @0xde913ebe8e1b82a5 {
outcome @0 :EventOutcome;
}

struct ScheduledRun @0xd98fc1ae5c8095d0 {
outcome @0 :EventOutcome;

Expand Down Expand Up @@ -404,7 +408,7 @@ interface EventDispatcher @0xf20697475ec1752d {
getHttpService @0 () -> (http :HttpService) $Cxx.allowCancellation;
# Gets the HTTP interface to this worker (to trigger FetchEvents).

sendTraces @1 (traces :List(Trace)) $Cxx.allowCancellation;
sendTraces @1 (traces :List(Trace)) -> (result :SendTracesRun) $Cxx.allowCancellation;
# Deliver a trace event to a trace worker. This always completes immediately; the trace handler
# runs as a "waitUntil" task.

Expand Down
2 changes: 1 addition & 1 deletion src/workerd/io/worker-interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class WorkerInterface: public kj::HttpService {
// for the promise, then invoke the destination object.
kj::Own<WorkerInterface> newPromisedWorkerInterface(
kj::TaskSet& waitUntilTasks, kj::Promise<kj::Own<WorkerInterface>> promise);
// TODO(cleanup): `waitUntilTasks` is only needed to handle `sendTraces` and `prewarm` since they
// TODO(cleanup): `waitUntilTasks` is only needed to handle `prewarm` since they
// don't return promises. We should maybe change them to return promises?

// Adapts WorkerInterface to HttpClient, including taking ownership.
Expand Down

0 comments on commit 0d58827

Please sign in to comment.