Merge pull request #5252 from cloudflare/mar/stw-r2-checksum-dup

Fix R2 tracing to use unique tags for each checksum type instead of overwriting shared tags

Author: mar-cf

src/workerd/api/r2-bucket.c++

@@ -328,27 +328,22 @@ void addHeadResultSpanTags(
       kj::str(toISOString(js, headResult.getUploaded()).asPtr()));
   auto checksums = headResult.getChecksums();
   KJ_IF_SOME(md5, checksums.get()->md5) {
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.value"_kjc, kj::encodeHex(md5));
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.type"_kjc, kj::str("md5"));
+    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.md5"_kjc, kj::encodeHex(md5));
   }
   KJ_IF_SOME(sha1, checksums.get()->sha1) {
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.value"_kjc, kj::encodeHex(sha1));
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.type"_kjc, kj::str("sha1"));
+    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.sha1"_kjc, kj::encodeHex(sha1));
   }
   KJ_IF_SOME(sha256, checksums.get()->sha256) {
     traceContext.userSpan.setTag(
-        "cloudflare.r2.response.checksum.value"_kjc, kj::encodeHex(sha256));
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.type"_kjc, kj::str("sha256"));
+        "cloudflare.r2.response.checksum.sha256"_kjc, kj::encodeHex(sha256));
   }
   KJ_IF_SOME(sha384, checksums.get()->sha384) {
     traceContext.userSpan.setTag(
-        "cloudflare.r2.response.checksum.value"_kjc, kj::encodeHex(sha384));
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.type"_kjc, kj::str("sha384"));
+        "cloudflare.r2.response.checksum.sha384"_kjc, kj::encodeHex(sha384));
   }
   KJ_IF_SOME(sha512, checksums.get()->sha512) {
     traceContext.userSpan.setTag(
-        "cloudflare.r2.response.checksum.value"_kjc, kj::encodeHex(sha512));
-    traceContext.userSpan.setTag("cloudflare.r2.response.checksum.type"_kjc, kj::str("sha512"));
+        "cloudflare.r2.response.checksum.sha512"_kjc, kj::encodeHex(sha512));
   }
 
   traceContext.userSpan.setTag(

src/workerd/api/r2-instrumentation-test.js

@@ -1077,7 +1077,7 @@ export const test = {
         'cloudflare.binding.name': 'BUCKET',
         'cloudflare.r2.operation': 'PutObject',
         'cloudflare.r2.bucket': 'r2-test',
-        'cloudflare.r2.request.key': 'md5checksum',
+        'cloudflare.r2.request.key': 'multipleChecksums',
         'cloudflare.r2.request.checksum.type': 'md5',
         'cloudflare.r2.request.checksum.value':
           '9a0364b9e99bb480dd25e1f0284c8555',
@@ -1086,6 +1086,33 @@ export const test = {
         'cloudflare.r2.response.etag': 'objectEtag',
         'cloudflare.r2.response.size': 123,
         'cloudflare.r2.response.uploaded': '2024-08-27T14:00:57.918Z',
+        'cloudflare.r2.response.checksum.md5':
+          '9a0364b9e99bb480dd25e1f0284c8555',
+        'cloudflare.r2.response.checksum.sha1':
+          '2a0364b9e99bb480dd25e1f0284c855511223344',
+        'cloudflare.r2.response.checksum.sha256':
+          '3a0364b9e99bb480dd25e1f0284c8555112233445566778899aabbccddeeff00',
+        'cloudflare.r2.response.storage_class': 'Standard',
+        'cloudflare.r2.response.custom_metadata': true,
+        closed: true,
+      },
+      {
+        name: 'r2_head',
+        'cloudflare.binding.type': 'r2',
+        'cloudflare.binding.name': 'BUCKET',
+        'cloudflare.r2.operation': 'HeadObject',
+        'cloudflare.r2.bucket': 'r2-test',
+        'cloudflare.r2.request.key': 'multipleChecksums',
+        'cloudflare.r2.response.success': true,
+        'cloudflare.r2.response.etag': 'objectEtag',
+        'cloudflare.r2.response.size': 123,
+        'cloudflare.r2.response.uploaded': '2024-08-27T14:00:57.918Z',
+        'cloudflare.r2.response.checksum.md5':
+          '9a0364b9e99bb480dd25e1f0284c8555',
+        'cloudflare.r2.response.checksum.sha1':
+          '2a0364b9e99bb480dd25e1f0284c855511223344',
+        'cloudflare.r2.response.checksum.sha256':
+          '3a0364b9e99bb480dd25e1f0284c8555112233445566778899aabbccddeeff00',
         'cloudflare.r2.response.storage_class': 'Standard',
         'cloudflare.r2.response.custom_metadata': true,
         closed: true,

src/workerd/api/r2-test.js

@@ -45,6 +45,17 @@ const md5Buffer = new Uint8Array([
   0x9a, 0x03, 0x64, 0xb9, 0xe9, 0x9b, 0xb4, 0x80, 0xdd, 0x25, 0xe1, 0xf0, 0x28,
   0x4c, 0x85, 0x55,
 ]);
+// Test SHA1 checksum
+const sha1Buffer = new Uint8Array([
+  0x2a, 0x03, 0x64, 0xb9, 0xe9, 0x9b, 0xb4, 0x80, 0xdd, 0x25, 0xe1, 0xf0, 0x28,
+  0x4c, 0x85, 0x55, 0x11, 0x22, 0x33, 0x44,
+]);
+// Test SHA256 checksum
+const sha256Buffer = new Uint8Array([
+  0x3a, 0x03, 0x64, 0xb9, 0xe9, 0x9b, 0xb4, 0x80, 0xdd, 0x25, 0xe1, 0xf0, 0x28,
+  0x4c, 0x85, 0x55, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa,
+  0xbb, 0xcc, 0xdd, 0xee, 0xff, 0x00,
+]);
 const objResponse = {
   name: key,
   version: 'objectVersion',
@@ -359,10 +370,18 @@ export default {
               });
             }
           }
-          case 'md5checksum': {
+          case 'multipleChecksums': {
+            const toHex = (buffer) =>
+              Array.from(buffer, (b) => b.toString(16).padStart(2, '0')).join(
+                ''
+              );
             return Response.json({
               ...objResponse,
-              md5: md5Buffer,
+              checksums: {
+                0: toHex(md5Buffer), // md5
+                1: toHex(sha1Buffer), // sha1
+                2: toHex(sha256Buffer), // sha256
+              },
             });
           }
         }
@@ -554,6 +573,22 @@ export default {
               body,
             });
           }
+          case 'multipleChecksums': {
+            const toHex = (buffer) =>
+              Array.from(buffer, (b) => b.toString(16).padStart(2, '0')).join(
+                ''
+              );
+            return buildGetResponse({
+              head: {
+                checksums: {
+                  0: toHex(md5Buffer), // md5
+                  1: toHex(sha1Buffer), // sha1
+                  2: toHex(sha256Buffer), // sha256
+                },
+              },
+              body: jsonRequest.method === 'get' ? body : undefined,
+            });
+          }
         }
         throw new Error('Unexpected GET');
       }
@@ -893,11 +928,15 @@ export default {
     }
     // Checksums
     {
-      // This exists purely to test the instrumentation
-      let resp = await env.BUCKET.put('md5checksum', body, {
+      // This tests the instrumentation with multiple checksums to ensure proper tag handling
+      let resp = await env.BUCKET.put('multipleChecksums', body, {
         md5: md5Buffer,
       });
       assert.ok(resp);
+
+      // Also test HEAD operation to verify checksum tags
+      const headResp = await env.BUCKET.head('multipleChecksums');
+      assert.ok(headResp);
     }
   },
 };