Skip to content

R2 Instrumentation: Move the spans up a level in the call stack#4950

Merged
jmorrell-cloudflare merged 6 commits intomainfrom
jmorrell/r2-move-spans
Sep 2, 2025
Merged

R2 Instrumentation: Move the spans up a level in the call stack#4950
jmorrell-cloudflare merged 6 commits intomainfrom
jmorrell/r2-move-spans

Conversation

@jmorrell-cloudflare
Copy link
Copy Markdown
Contributor

@jmorrell-cloudflare jmorrell-cloudflare commented Sep 2, 2025

Depends on #4949

The spans are currently created where the information we want to annotate them with is not available. This moves them up so that they are created inline with the method they are instrumenting. This is similar to what we've done in the KV module. This also adopts @danlapid's changes in managing span lifetimes from #4861

@jmorrell-cloudflare jmorrell-cloudflare requested review from a team as code owners September 2, 2025 03:37
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 2, 2025

The generated output of @cloudflare/workers-types matches the snapshot in types/generated-snapshot 🎉

@jmorrell-cloudflare jmorrell-cloudflare marked this pull request as draft September 2, 2025 03:47
@jmorrell-cloudflare jmorrell-cloudflare marked this pull request as ready for review September 2, 2025 04:21
Copy link
Copy Markdown
Collaborator

@danlapid danlapid left a comment

Choose a reason for hiding this comment

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

I don't know if I love the span names or the attribute names but I dislike bike-shedding on names.
The code looks correct.

Base automatically changed from jmorrell/r2-add-instrumentation-test to main September 2, 2025 17:20
The spans are currently created where the information we want to
annotate them with is not available. This moves them up so that they are
created inline with the method they are instrumenting. This is similar
to what we've done in the KV module.
Nope, this is used by edgeworker. I will modify those in a separate
change.
@jmorrell-cloudflare jmorrell-cloudflare merged commit 6a92f19 into main Sep 2, 2025
21 of 22 checks passed
@jmorrell-cloudflare jmorrell-cloudflare deleted the jmorrell/r2-move-spans branch September 2, 2025 18:20
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.

2 participants