JSRPC: Fill in trace (tail) events better. #1895
Conversation
|
|
||
| // We call this `rpcMethod` to make clear this is an RPC event, since some tail workers rely | ||
| // on duck-typing EventInfo based on the properties present. (`methodName` might be ambiguous | ||
| // since HTTP also has methods.) |
There was a problem hiding this comment.
From the comment, I assume that if it weren't for these details of tail workers or the JS API, we'd prefer to call the property methodName as elsewhere in the code, such that it isn't worth propagating the rpcMethod name to the analogous Trace::JsRpcEventInfo::methodName field or beyond.
There was a problem hiding this comment.
Well, I had written all the code before I realized that the tail worker distinguishes event types based on what properties are present. At that point I decided only to change the JS API. The other places could be renamed in the future without breaking any kind of compatibility, but I think they are fine as-is.
|
|
||
| switch (callParams.which()) { | ||
| case rpc::JsRpcTarget::CallParams::METHOD_NAME: { | ||
| kj::StringPtr methodName = callParams.getMethodName(); | ||
| result = getProperty(methodName); | ||
| methodNameForErrors = methodName; | ||
| methodNameForTrace = methodName.attach(); |
There was a problem hiding this comment.
If I'm understanding correctly, this StringPtr::attach() call produces a ConstString that does not own its own memory, but instead relies on the caller to keep callParams alive.
...which I assume is OK, since the only caller of tryGetProperty() uses the returned methodNameForTrace value synchronously in the same scope as the passed params value.
| EmailEventInfo, TraceEventInfo, HibernatableWebSocketEventInfo, CustomEventInfo> EventInfo; | ||
| typedef kj::OneOf<FetchEventInfo, JsRpcEventInfo, ScheduledEventInfo, AlarmEventInfo, | ||
| QueueEventInfo, EmailEventInfo, TraceEventInfo, HibernatableWebSocketEventInfo, | ||
| CustomEventInfo> EventInfo; | ||
| kj::Maybe<EventInfo> eventInfo; | ||
| // TODO(someday): Support more event types. |
There was a problem hiding this comment.
Heh... Guess we can remove this comment, since we now support lots of event types.
kentonv
force-pushed
the
kenton/jsrpc-tracing
branch
2 times, most recently
from
1386762
to
d0dd6d4
Compare
eventInfo was being left null, causing wrangler tail not to report the event at all. Let's fill it in and specify the top-level method name.
Well, this isn't really specific to JSRPC, but JSRPC will use named entrypoints more.
kentonv
force-pushed
the
kenton/jsrpc-tracing
branch
from
d0dd6d4
to
8f47fb2
Compare
eventInfo was being left null, causing wrangler tail not to report the event at all. Let's fill it in and specify the top-level method name.
Also added entrypoint name, which seems highly relevant to JSRPC.