Skip to content
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 KV span tags #3261

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

fhanau
Copy link
Collaborator

@fhanau fhanau commented Dec 20, 2024

#3256 needs to land first.

@fhanau fhanau requested a review from mikea December 20, 2024 01:25
@fhanau fhanau requested review from a team as code owners December 20, 2024 01:25
@fhanau fhanau force-pushed the felix/subrequest-client-spans-kv branch from 04c7f43 to 076a736 Compare December 20, 2024 01:31
jsg::Promise<GetResult> get(jsg::Lock& js,
kj::String name,
jsg::Optional<kj::OneOf<kj::String, GetOptions>> options,
CompatibilityFlags::Reader flags);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor cleanup: compat flags parameter is not needed here, that should simplify the generated code slightly.

@fhanau fhanau force-pushed the felix/subrequest-client-spans-kv branch from 076a736 to 0d0a53f Compare December 20, 2024 15:59
@fhanau fhanau force-pushed the felix/subrequest-client-spans-kv branch from 0d0a53f to e182c52 Compare December 31, 2024 00:14
Copy link
Collaborator

@mikea mikea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sequences of very similar literals are not the best, otherwise lg.

@@ -439,7 +460,8 @@ jsg::Promise<kj::Maybe<jsg::Ref<R2Bucket::HeadResult>>> R2Bucket::put(jsg::Lock&
});

auto& context = IoContext::current();
auto client = context.getHttpClient(clientIndex, true, kj::none, "r2_put"_kjc);
auto client = r2GetClient(context, clientIndex, "r2_put"_kjc, "PutObject"_kjc,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think that calling the original function with initializer in-place wouldn't increase code amount much (maybe couple lines in total), but would be much more readable:

auto client = r2GetClient(context, clientIndex, { { "rpc.method"_kjc, "PutObject"_kjc} ... }

because currrently it is very hard to tell what is r2_put and what is PutObject and what is the difference.

@fhanau fhanau force-pushed the felix/subrequest-client-spans-kv branch from e182c52 to b48a847 Compare December 31, 2024 19:09
Copy link

The generated output of @cloudflare/workers-types has been changed by this PR. If this is intentional, run bazel build //types && rm -rf types/generated-snapshot && cp -r bazel-bin/types/definitions types/generated-snapshot to update the snapshot. Alternatively, you can download the full generated types: https://github.com/cloudflare/workerd/actions/runs/12563104814/artifacts/2374960853

Full Type Diff
diff -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";

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants