-
Notifications
You must be signed in to change notification settings - Fork 698
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
Synchronize observability settings during versions deploy #7091
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cab588b The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-wrangler-7091 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7091/npm-package-wrangler-7091 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-wrangler-7091 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-create-cloudflare-7091 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-kv-asset-handler-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-miniflare-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-pages-shared-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-vitest-pool-workers-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-workers-editor-shared-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-workers-shared-7091 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/11522361067/npm-package-cloudflare-workflows-shared-7091 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
) { | ||
const maybeUndefinedSettings = { | ||
logpush: config.logpush, | ||
tail_consumers: config.tail_consumers, | ||
observability: config.observability, // TODO reconcile with how regular deploy handles empty state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the desired end state here when it's empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. When @RamIdeas added versions-deploy, any undefined settings are not synchronized. When @zebp added observability to regular deploy, it defaults to false when undefined.
It would be unfortunate to need to always patch settings to false for undefined observabililty, but that is the simplest way to reconcile them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the default is false then how about we remove the default (make it undefined, i.e. falsey) so this just works™️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RamIdeas I'm not sure I follow. The default does seem to be undefined/falsey, but the difference between deploy and versions-deploy is that deploy always patches the full NonVersionedScriptSettings whereas versions-deploy only does so if at least one field is defined.
So deploy with undefined observability will disable it, whereas versions-deploy will leave it unchanged. Should we always patch during versions-deploy? Or conditionally patch during deploy? Or leave them slightly different?
1dfe8b0
to
2f56dcf
Compare
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.
2f56dcf
to
cab588b
Compare
│ | ||
│ Synced non-versioned settings: | ||
│ logpush: <skipped> | ||
│ observability: enabled: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we display head_sampling_rate with <skipped>
if it's not present? to avoid this single-entry nested value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to go with whatever you suggest, but I would personally find that confusing. logpush: <skipped>
means that wrangler left the remote state unchanged. Omitting head_sampling_rate does not skip synchronizing that field, but causes the default behavior of 100% sampling, which may be different from the prior setting.
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.