From cab588bc6023cf1ece40c5176064a215a56230e7 Mon Sep 17 00:00:00 2001
From: Taylor Lee <tlee@cloudflare.com>
Date: Thu, 24 Oct 2024 12:48:08 -0700
Subject: [PATCH] fix: sync observability on versions deploy

In addition to logpush and tail_consumers, patch observability settings
on version deploy. This still doesn't quite match the behavior of
"wrangler deploy", which will disable observability if it is not defined
in wrangler.toml (as of #6776), but at least this is more correct for now.
---
 .changeset/chilled-apes-repeat.md             |  7 ++
 .../versions/versions.deploy.test.ts          | 67 ++++++++++++++++++-
 packages/wrangler/src/versions/deploy.ts      | 15 ++++-
 3 files changed, 86 insertions(+), 3 deletions(-)
 create mode 100644 .changeset/chilled-apes-repeat.md

diff --git a/.changeset/chilled-apes-repeat.md b/.changeset/chilled-apes-repeat.md
new file mode 100644
index 000000000000..4f15f093c5fe
--- /dev/null
+++ b/.changeset/chilled-apes-repeat.md
@@ -0,0 +1,7 @@
+---
+"wrangler": patch
+---
+
+fix: synchronize observability settings during `wrangler versions deploy`
+
+When running `wrangler versions deploy`, Wrangler will now update `observability` settings in addition to `logpush` and `tail_consumers`. Unlike `wrangler deploy`, it will not disable observability when `observability` is undefined in `wrangler.toml`.
diff --git a/packages/wrangler/src/__tests__/versions/versions.deploy.test.ts b/packages/wrangler/src/__tests__/versions/versions.deploy.test.ts
index c4b385f057d6..63e6d1f90235 100644
--- a/packages/wrangler/src/__tests__/versions/versions.deploy.test.ts
+++ b/packages/wrangler/src/__tests__/versions/versions.deploy.test.ts
@@ -704,17 +704,78 @@ describe("versions deploy", () => {
 				│
 				│ Synced non-versioned settings:
 				│            logpush:  true
+				│      observability:  <skipped>
 				│     tail_consumers:  <skipped>
 				│
 				╰  SUCCESS  Deployed test-name version 00000000-0000-0000-0000-000000000000 at 100% (TIMINGS)"
 			`);
+		});
+
+		test("with observability disabled in wrangler.toml", async () => {
+			writeWranglerToml({
+				observability: {
+					enabled: false,
+				},
+			});
 
-			expect(normalizeOutput(std.out)).toContain("logpush:  true");
+			const result = runWrangler(
+				"versions deploy 10000000-0000-0000-0000-000000000000 --yes --experimental-gradual-rollouts"
+			);
+
+			await expect(result).resolves.toBeUndefined();
+
+			expect(normalizeOutput(std.out)).toMatchInlineSnapshot(`
+				"╭ Deploy Worker Versions by splitting traffic between multiple versions
+				│
+				├ Fetching latest deployment
+				│
+				├ Your current deployment has 2 version(s):
+				│
+				│ (10%) 00000000-0000-0000-0000-000000000000
+				│       Created:  TIMESTAMP
+				│           Tag:  -
+				│       Message:  -
+				│
+				│ (90%) 00000000-0000-0000-0000-000000000000
+				│       Created:  TIMESTAMP
+				│           Tag:  -
+				│       Message:  -
+				│
+				├ Fetching deployable versions
+				│
+				├ Which version(s) do you want to deploy?
+				├ 1 Worker Version(s) selected
+				│
+				├     Worker Version 1:  00000000-0000-0000-0000-000000000000
+				│              Created:  TIMESTAMP
+				│                  Tag:  -
+				│              Message:  -
+				│
+				├ What percentage of traffic should Worker Version 1 receive?
+				├ 100% of traffic
+				├
+				├ Add a deployment message (skipped)
+				│
+				├ Deploying 1 version(s)
+				│
+				├ Syncing non-versioned settings
+				│
+				│ Synced non-versioned settings:
+				│            logpush:  <skipped>
+				│      observability:  enabled:  false
+				│     tail_consumers:  <skipped>
+				│
+				╰  SUCCESS  Deployed test-name version 00000000-0000-0000-0000-000000000000 at 100% (TIMINGS)"
+			`);
 		});
 
-		test("with logpush and tail_consumers in wrangler.toml", async () => {
+		test("with logpush, tail_consumers, and observability in wrangler.toml", async () => {
 			writeWranglerToml({
 				logpush: false,
+				observability: {
+					enabled: true,
+					head_sampling_rate: 0.5,
+				},
 				tail_consumers: [
 					{ service: "worker-1" },
 					{ service: "worker-2", environment: "preview" },
@@ -766,6 +827,8 @@ describe("versions deploy", () => {
 				│
 				│ Synced non-versioned settings:
 				│            logpush:  false
+				│      observability:  enabled:             true
+				│                      head_sampling_rate:  0.5
 				│     tail_consumers:  worker-1
 				│                      worker-2 (preview)
 				│                      worker-3 (staging)
diff --git a/packages/wrangler/src/versions/deploy.ts b/packages/wrangler/src/versions/deploy.ts
index 37df47e550eb..5f195141f5fa 100644
--- a/packages/wrangler/src/versions/deploy.ts
+++ b/packages/wrangler/src/versions/deploy.ts
@@ -563,7 +563,7 @@ async function maybePatchSettings(
 	const maybeUndefinedSettings = {
 		logpush: config.logpush,
 		tail_consumers: config.tail_consumers,
-		observability: config.observability,
+		observability: config.observability, // TODO reconcile with how regular deploy handles empty state
 	};
 	const definedSettings = Object.fromEntries(
 		Object.entries(maybeUndefinedSettings).filter(
@@ -588,9 +588,22 @@ async function maybePatchSettings(
 		},
 	});
 
+	const observability: Record<string, string> = {};
+	if (patchedSettings.observability) {
+		observability["enabled"] = String(patchedSettings.observability.enabled);
+		if (patchedSettings.observability.head_sampling_rate) {
+			observability["head_sampling_rate"] = String(
+				patchedSettings.observability.head_sampling_rate
+			);
+		}
+	}
 	const formattedSettings = formatLabelledValues(
 		{
 			logpush: String(patchedSettings.logpush ?? "<skipped>"),
+			observability:
+				Object.keys(observability).length > 0
+					? formatLabelledValues(observability)
+					: "<skipped>",
 			tail_consumers:
 				patchedSettings.tail_consumers
 					?.map((tc) =>