-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[o11y] Add remaining R2 span tags #3256
Conversation
The generated output of Full Type Diffdiff -r bazel-bin/types/definitions/experimental/index.d.ts types/generated-snapshot/experimental/index.d.ts
1680c1680
< cache?: "no-store" | "no-cache";
---
> cache?: "no-store";
1694c1694
< cache?: "no-store" | "no-cache";
---
> cache?: "no-store";
diff -r bazel-bin/types/definitions/experimental/index.ts types/generated-snapshot/experimental/index.ts
1688c1688
< cache?: "no-store" | "no-cache";
---
> cache?: "no-store";
1702c1702
< cache?: "no-store" | "no-cache";
---
> cache?: "no-store"; |
24366c3
to
f4b9130
Compare
598986f
to
956a297
Compare
f4b9130
to
a8d937b
Compare
src/workerd/api/r2-bucket.c++
Outdated
@@ -24,6 +24,25 @@ | |||
#include <regex> | |||
|
|||
namespace workerd::api::public_beta { | |||
kj::Own<kj::HttpClient> r2GetClient(IoContext& context, | |||
uint subrequestChannel, | |||
kj::LiteralStringConst op, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that defining a simple struct UserTracing
or something would go a long way for readability above (due to .method = "r2_delete"_kjc
) and below because of maybe casts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I understand correctly – I assume you mean a struct like this:
struct UserTracing {
kj::LiteralStringConst op;
kj::LiteralStringConst method;
kj::Maybe<kj::StringPtr> bucket;
kj::Maybe<StringTagParams> extraTag;
};
Then we could pass parameters simply as {"r2_get"_kjc, "GetObject"_kjc, this->adminBucketName()}
, but you prefer using this instead?
{
.op = "r2_get"_kjc,
.method = "GetObject"_kjc,
.bucket = this->adminBucketName()
}
FWIW I was able to clean up the Maybe<StringTagParams> wrappers in the last push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the version with names spelled out, yes. It is much easier to read and understand how r2_get
is different from GetObject
d368098
to
c033b70
Compare
src/workerd/api/r2-bucket.c++
Outdated
@@ -24,6 +24,25 @@ | |||
#include <regex> | |||
|
|||
namespace workerd::api::public_beta { | |||
kj::Own<kj::HttpClient> r2GetClient(IoContext& context, | |||
uint subrequestChannel, | |||
kj::LiteralStringConst op, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer the version with names spelled out, yes. It is much easier to read and understand how r2_get
is different from GetObject
3a95a73
to
13135a2
Compare
Merged via #3261. |
Please consult the (internal) span definitions to review the added span tags. Input is welcome on API design for
r2GetClient()
andgetHttpClientWithCallback()
.