Add error_message field to bundle deploy telemetry#4793
Open
shreyas-goenka wants to merge 16 commits intomainfrom
Open
Add error_message field to bundle deploy telemetry#4793shreyas-goenka wants to merge 16 commits intomainfrom
shreyas-goenka wants to merge 16 commits intomainfrom
Conversation
c7ee5c8 to
cb59f1d
Compare
Collaborator
|
Commit: 4919773
29 interesting tests: 12 flaky, 10 SKIP, 7 KNOWN
Top 20 slowest tests (at least 2 minutes):
|
Track the first error diagnostic summary encountered during bundle deploy in telemetry. Move telemetry logging into a defer so it's always captured, even when deploy fails. Co-authored-by: Isaac
cb59f1d to
1a8ff40
Compare
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @pietern, @simonfaltum Suggestions based on git history of 13 changed files (6 scored). See CODEOWNERS for path-specific ownership rules. |
simonfaltum
reviewed
Mar 22, 2026
Member
simonfaltum
left a comment
There was a problem hiding this comment.
Review Swarm Summary (2 independent reviewers + cross-review)
Verdict: REQUEST CHANGES - The defer-based approach is correct in principle, but has 3 issues that need attention before merge.
- [IMPORTANT] Potential panic on auth/config failures - The deferred
LogDeployTelemetry()runs wheneverb != nil, even beforecmdctx.SetConfigUsed()succeeds. On auth/config failures, telemetry upload will callcmdctx.ConfigUsed(ctx)and panic. - [IMPORTANT] Double deploy events on failure - Failed deploys will emit two
BundleDeployEvents: one from the new defer and one from the root-level fallback incmd/root/root.go:185. - [IMPORTANT] PII risk in error messages -
ErrorMessageis sent verbatim with no sanitization or size bound. Many deploy errors interpolate local filesystem paths, resource names, and workspace URLs.
Two fixes: 1. Remove redundant empty BundleDeployEvent logging in root.go Execute. The defer in process.go already logs detailed deploy telemetry via LogDeployTelemetry on all exit paths. The root.go code caused duplicate events and Upload retries (mock returns numProtoSuccess=1). 2. Add HasConfigUsed guard in telemetry.Upload before calling ConfigUsed. When bundle config fails early (e.g., restricted script execution blocking preinit), ConfigUsed is never set on the context. The LogDeployTelemetry defer still logs an event, so Upload would panic trying to create an API client without auth config. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
93f4e62 to
a2fb5dd
Compare
426f35a to
e50f480
Compare
e50f480 to
b6d60a8
Compare
b6d60a8 to
36d67ed
Compare
36d67ed to
7b76e5b
Compare
Adds a scrubber that runs before error messages are sent to telemetry: 1. Replaces the bundle root path with "." to avoid leaking local paths 2. Redacts remaining home directory paths (/Users/..., /home/..., C:\Users\...) 3. Redacts email addresses (e.g., in workspace paths) Inspired by VS Code's telemetry path scrubbing and Sentry's @userpath rule. Co-authored-by: Isaac
7b76e5b to
ca229c4
Compare
… path redaction - Replace windowsHomeDirRegexp with general windowsAbsPathRegexp - Add workspacePathRegexp for [REDACTED_WORKSPACE_PATH] label - Use capturing groups instead of replaceDelimitedMatch helper - Merge HomeDirRegexFallback tests into AbsolutePaths test Co-authored-by: Isaac
… test Co-authored-by: Isaac
The replacePath function converted bundle root to "." and home dir to "~" before regex scrubbing. Since absolute and relative paths are handled by separate regex patterns anyway, this added complexity for no benefit. Absolute paths now consistently get [REDACTED_PATH]. Co-authored-by: Isaac
~/... expands to an absolute path, so it should match [REDACTED_PATH] rather than [REDACTED_REL_PATH]. Co-authored-by: Isaac
Redacted paths now include the file extension for a known set of types, e.g. [REDACTED_PATH](yml), [REDACTED_REL_PATH](json). This helps with debugging without leaking sensitive information. Co-authored-by: Isaac
Add .r, .scala, .sh, .hcl, .ini, .zip, .tar, .csv for better coverage of file types common in the Databricks CLI ecosystem. Co-authored-by: Isaac
Add .js, .ts, .jsx, .tsx, .html, .css (Databricks Apps / Vite), .env, and .md. Co-authored-by: Isaac
Add .xml, .properties, .conf, .tfstate, .tfvars, .egg, .gz, .tgz, .dbc, .parquet, .avro, .log, .lock, .pem, .crt. Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
Adds an
error_messagefield tobundle deploytelemetry events, giving visibility into user-facing deploy errors.Error capture
LogDeployTelemetryis now called from adeferinProcessBundleRet, so it runs on all exit paths — both diagnostic errors (vialogdiag.GetFirstErrorSummary) and regular Go errors. The legacy emptyBundleDeployEventincmd/root/root.gois removed; theHasConfigUsedauth guard is now intelemetry.Uploadso it applies to all telemetry.Scrubbing
Error messages are treated as PII by the logging infrastructure. Before including them in telemetry, a best-effort regex scrubber (
telemetry_scrub.go) redacts paths and emails to avoid collecting more information than necessary:[REDACTED_PATH]/home/user/project/file.yml→[REDACTED_PATH](yml)~/...paths[REDACTED_PATH]~/.databricks/config.json→[REDACTED_PATH](json)/Workspace/...paths[REDACTED_WORKSPACE_PATH]/Workspace/Users/dev/.bundle→[REDACTED_WORKSPACE_PATH][REDACTED_PATH]C:\Users\...\file.yml→[REDACTED_PATH](yml)[REDACTED_REL_PATH]./resources/job.yml→[REDACTED_REL_PATH](yml)[REDACTED_EMAIL]user@example.com→[REDACTED_EMAIL]Known file extensions (
.yml,.json,.py,.tf, etc.) are preserved in parentheses to help understand usage patterns without capturing sensitive information. Messages are capped at 500 characters.Tests
telemetry_scrub_test.go)bundle/telemetry/deploy-error-message)