[o11y] Add rudimentary user tracing support for connect() #3089
Conversation
fhanau
commented
Nov 11, 2024
fhanau
commented
- Use more descriptive name for IOContext function
src/workerd/api/sockets.c++
Outdated
| @@ -180,6 +180,7 @@ jsg::Ref<Socket> connectImplNoOutputLock(jsg::Lock& js, | |||
|
|
|||
| auto& ioContext = IoContext::current(); | |||
| JSG_REQUIRE(!ioContext.isFiddle(), TypeError, "Socket API not supported in web preview mode."); | |||
| ioContext.makeUserTraceSpan("connect"_kjc); | |||
There was a problem hiding this comment.
I imagine it's intentional that we're dropping the return value immediately and not measuring anything about the duration of the connect?
It looks a little funny, but so long as it's by design the code LGTM.
There was a problem hiding this comment.
I think you're right – we want the span to exist for the entire duration of the function, and I guess if no variable is declared for it, it goes out of scope immediately instead of at the end of the function. In some other cases we need to assign the returned span so that we can extend its lifetime for async I/O or to create subspans, but I guess this is still needed here too.
There was a problem hiding this comment.
@mikea would this make makeUserTraceSpan() and similarly SpanBuilder::newChild() good candidates for adding the [[nodiscard]] to avoid this mistake in the future?
There was a problem hiding this comment.
looks like [[nodiscard]] is a must for them.
- Use more descriptive name for IOContext function
fhanau
force-pushed
the
felix/user-tracing-connect
branch
from
f1e8ab1 to
cf6cf6e
Compare
If newly created spans are immediately discarded, they will not be able to capture the run time of the underlying operation.
* [o11y] Add rudimentary user tracing support for connect() - Use more descriptive name for IOContext function * Add [[nodiscard]] attribute to span creation functions If newly created spans are immediately discarded, they will not be able to capture the run time of the underlying operation.
