Skip to content

Commit

Permalink
fix: sync observability on versions deploy
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
taylorlee committed Oct 25, 2024
1 parent 6063882 commit cab588b
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 3 deletions.
7 changes: 7 additions & 0 deletions .changeset/chilled-apes-repeat.md
Original file line number Diff line number Diff line change
@@ -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`.
67 changes: 65 additions & 2 deletions packages/wrangler/src/__tests__/versions/versions.deploy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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" },
Expand Down Expand Up @@ -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)
Expand Down
15 changes: 14 additions & 1 deletion packages/wrangler/src/versions/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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) =>
Expand Down

0 comments on commit cab588b

Please sign in to comment.