First pass at instrumenting kv.list #4361
Conversation
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| }); | ||
| } | ||
|
|
||
| constexpr auto FLPROD_405_HEADER = "CF-KV-FLPROD-405"_kj; | ||
|
|
||
| kj::Own<kj::HttpClient> KvNamespace::getHttpClient(IoContext& context, |
There was a problem hiding this comment.
The other KvNamespace::getHttpClient creates spans for every method all at once. This works only if you do not want to introspect the response or capture errors during execution.
This implementation allows us more flexibility with instrumentation. I will move each KV method over to it as I instrument them and then delete the other.
jmorrell-cloudflare
force-pushed
the
jmorrell/kv-instrumentation-2
branch
2 times, most recently
from
d484a38 to
b71234d
Compare
jmorrell-cloudflare
force-pushed
the
jmorrell/kv-instrumentation-2
branch
from
b71234d to
7497317
Compare
| url.query.add(kj::Url::QueryParam{kj::str("prefix"), kj::str(prefix)}); | ||
| } | ||
| } | ||
| KJ_IF_SOME(maybeCursor, o.cursor) { | ||
| KJ_IF_SOME(cursor, maybeCursor) { | ||
| traceContext.userSpan.setTag("cloudflare.kv.query.cursor"_kjc, true); |
There was a problem hiding this comment.
Doesn't this overlap with the existing cloudflare.kv.query.parameter.cursor tag? Same applies for some related tags.
There was a problem hiding this comment.
See my other comment. The existing code trying to add all the instrumentation code in one place is less flexible given that it cannot observe anything about the response. The idea is to lift all of the instrumentation to live alongside the code it instruments.
| auto userSpan = context.makeUserTraceSpan("kv.list"_kjc); | ||
| TraceContext traceContext(kj::mv(traceSpan), kj::mv(userSpan)); | ||
|
|
||
| traceContext.userSpan.setTag("db.system"_kjc, kj::str("cloudflare.kv"_kjc)); |
There was a problem hiding this comment.
I don't think this should be set within list itself – see the existing getHttpClient(), where we set this attribute for a number of operations at once.
The value of the tag is not matching what we have there (cloudflare.kv vs. cloudflare-kv, for the operation name we have kv.list vs. kv_list)
There was a problem hiding this comment.
Yes, this modifies the existing instrumentation and extends it quite a bit: https://wiki.cfdata.org/display/WOBS/June+2025%3A+Next+steps+for+Runtime+Instrumentation
There are no dependencies on this existing instrumentation that I'm aware of. Am I missing something?
There was a problem hiding this comment.
The naming scheme should always be consistent, everything else is likely to cause confusion, especially when renaming things gets forgotten until code is used in production. I'm not opposed to changing the naming scheme per se, but this should be done for all KV operations, in a commit or PR either before or after adding new tags. Everything else is difficult to review.
We also need to be mindful of keeping a consistent source of truth for this – both the old source of truth and the new document you added use db.system:cloudflare-kv, the tag implementation needs to be consistent with its specification.
|
|
||
| traceContext.userSpan.setTag("db.system"_kjc, kj::str("cloudflare.kv"_kjc)); | ||
| traceContext.userSpan.setTag("db.operation.name"_kjc, kj::str("list"_kjc)); | ||
| traceContext.userSpan.setTag("cloudflare.binding_type"_kjc, kj::str("KV"_kjc)); |
There was a problem hiding this comment.
If we add this we'd want to have it for each binding, right? Not opposed to adding it per se, but that's something that should be discussed internally first so that we agree that it's needed/what the right tag name is, and it should be added across bindings in a different change set.
There was a problem hiding this comment.
I'm certainly open to renaming it, but we absolutely want some equivalent of this attribute for all of our bindings
There was a problem hiding this comment.
My point is that this is a KV instrumentation PR – adding "global" tags is out of scope here.
Note: depends on merging #4356 firstTraceContextcan be passed along, and modifieskv.listto use the new path.kv.listplus tests