Skip to content

Commit 3f7199b

Browse files
Add bundle deployment telemetry for variables, validation gap, and dependency graph
Add several new telemetry signals to improve understanding of DABs usage patterns: - Detect nested variable references (var-in-var patterns like ${var.foo_${var.bar}}) - Track variable defaults count, lookup type distribution, override sources (env var, file, CLI flag), and resolution depth - Track validation-pass-deploy-fail gap via "validation_passed" metric with deferred telemetry logging - Track direct engine deployment plan metrics: action types, resource counts, dependency graph depth/width - Remove unused telemetry fields: SetFields, TerraformStateSizeBytes, FromWebTerminal Co-authored-by: Isaac
1 parent faf0949 commit 3f7199b

File tree

20 files changed

+942
-38
lines changed

20 files changed

+942
-38
lines changed

acceptance/bundle/telemetry/deploy-compute-type/output.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ Deployment complete!
3737
"key": "python_wheel_wrapper_is_set",
3838
"value": false
3939
},
40+
{
41+
"key": "validation_passed",
42+
"value": true
43+
},
4044
{
4145
"key": "skip_artifact_cleanup",
4246
"value": false
@@ -79,6 +83,10 @@ Deployment complete!
7983
"key": "python_wheel_wrapper_is_set",
8084
"value": false
8185
},
86+
{
87+
"key": "validation_passed",
88+
"value": true
89+
},
8290
{
8391
"key": "skip_artifact_cleanup",
8492
"value": false

acceptance/bundle/telemetry/deploy-experimental/output.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ Deployment complete!
3636
"key": "python_wheel_wrapper_is_set",
3737
"value": false
3838
},
39+
{
40+
"key": "validation_passed",
41+
"value": true
42+
},
3943
{
4044
"key": "skip_artifact_cleanup",
4145
"value": false

acceptance/bundle/telemetry/deploy-name-prefix/custom/output.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ Deployment complete!
3232
"key": "python_wheel_wrapper_is_set",
3333
"value": false
3434
},
35+
{
36+
"key": "validation_passed",
37+
"value": true
38+
},
3539
{
3640
"key": "skip_artifact_cleanup",
3741
"value": false

acceptance/bundle/telemetry/deploy-name-prefix/mode-development/output.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ Deployment complete!
3232
"key": "python_wheel_wrapper_is_set",
3333
"value": false
3434
},
35+
{
36+
"key": "validation_passed",
37+
"value": true
38+
},
3539
{
3640
"key": "skip_artifact_cleanup",
3741
"value": false

acceptance/bundle/telemetry/deploy-whl-artifacts/output.txt

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ Deployment complete!
3636
"key": "python_wheel_wrapper_is_set",
3737
"value": false
3838
},
39+
{
40+
"key": "validation_passed",
41+
"value": true
42+
},
3943
{
4044
"key": "skip_artifact_cleanup",
4145
"value": false
@@ -80,6 +84,10 @@ Deployment complete!
8084
"key": "python_wheel_wrapper_is_set",
8185
"value": true
8286
},
87+
{
88+
"key": "validation_passed",
89+
"value": true
90+
},
8391
{
8492
"key": "skip_artifact_cleanup",
8593
"value": true

acceptance/bundle/telemetry/deploy/out.telemetry.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@
6666
"key": "python_wheel_wrapper_is_set",
6767
"value": false
6868
},
69+
{
70+
"key": "validation_passed",
71+
"value": true
72+
},
6973
{
7074
"key": "skip_artifact_cleanup",
7175
"value": false

bundle/bundle.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ type Metrics struct {
5656
PythonUpdatedResourcesCount int64
5757
ExecutionTimes []protos.IntMapEntry
5858
LocalCacheMeasurementsMs []protos.IntMapEntry // Local cache measurements stored as milliseconds
59+
DeployPlanMetrics []protos.IntMapEntry // Deployment plan and graph metrics (direct engine)
5960
}
6061

6162
// SetBoolValue sets the value of a boolean metric.

bundle/config/mutator/resolve_variable_references.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,11 +150,18 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
150150
// We rewrite it here to make the resolution logic simpler.
151151
varPath := dyn.NewPath(dyn.Key("var"))
152152

153+
// Detect nested variable references like ${var.foo_${var.bar}} and log
154+
// a telemetry event if found. These patterns are not supported by the
155+
// interpolation regex and silently fail to resolve.
156+
m.detectNestedVariableReferences(b)
157+
153158
var diags diag.Diagnostics
154159
maxRounds := 1 + m.extraRounds
160+
roundsUsed := 0
155161

156162
for round := range maxRounds {
157163
hasUpdates, newDiags := m.resolveOnce(b, prefixes, varPath)
164+
roundsUsed = round + 1
158165

159166
diags = diags.Extend(newDiags)
160167

@@ -180,6 +187,13 @@ func (m *resolveVariableReferences) Apply(ctx context.Context, b *bundle.Bundle)
180187
b.Metrics.SetBoolValue("artifacts_reference_used", true)
181188
}
182189

190+
if roundsUsed > 1 {
191+
b.Metrics.SetBoolValue("variable_resolution_rounds_gt_1", true)
192+
}
193+
if roundsUsed > 3 {
194+
b.Metrics.SetBoolValue("variable_resolution_rounds_gt_3", true)
195+
}
196+
183197
return diags
184198
}
185199

@@ -294,6 +308,30 @@ func (m *resolveVariableReferences) selectivelyMutate(b *bundle.Bundle, fn func(
294308
})
295309
}
296310

311+
// errNestedVarRefFound is a sentinel error used to short-circuit WalkReadOnly
312+
// once a nested variable reference is detected.
313+
var errNestedVarRefFound = errors.New("nested variable reference found")
314+
315+
// detectNestedVariableReferences walks the bundle configuration and checks for
316+
// nested variable references like ${var.foo_${var.bar}}. These patterns are not
317+
// supported and are tracked via telemetry to understand how common they are.
318+
func (m *resolveVariableReferences) detectNestedVariableReferences(b *bundle.Bundle) {
319+
err := dyn.WalkReadOnly(b.Config.Value(), func(_ dyn.Path, v dyn.Value) error {
320+
s, ok := v.AsString()
321+
if !ok {
322+
return nil
323+
}
324+
325+
if dynvar.ContainsNestedVariableReference(s) {
326+
return errNestedVarRefFound
327+
}
328+
return nil
329+
})
330+
if err == errNestedVarRefFound {
331+
b.Metrics.SetBoolValue("nested_var_reference_used", true)
332+
}
333+
}
334+
297335
func getAllKeys(root dyn.Value) ([]string, error) {
298336
var keys []string
299337

bundle/config/mutator/resolve_variable_references_test.go

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,46 @@ import (
66
"github.com/databricks/cli/bundle"
77
"github.com/databricks/cli/bundle/config"
88
"github.com/databricks/cli/bundle/config/resources"
9+
"github.com/databricks/cli/bundle/config/variable"
10+
"github.com/databricks/cli/libs/telemetry/protos"
911
"github.com/databricks/databricks-sdk-go/service/pipelines"
12+
"github.com/stretchr/testify/assert"
1013
"github.com/stretchr/testify/require"
1114
)
1215

16+
func TestResolveVariableReferencesDetectsNestedVarRef(t *testing.T) {
17+
b := &bundle.Bundle{
18+
Config: config.Root{
19+
Bundle: config.Bundle{
20+
Name: "${var.env_${var.region}}",
21+
},
22+
},
23+
}
24+
25+
diags := bundle.Apply(t.Context(), b, ResolveVariableReferencesWithoutResources())
26+
// The nested reference won't resolve, but we should still detect it.
27+
_ = diags
28+
29+
assert.Contains(t, b.Metrics.BoolValues, protos.BoolMapEntry{Key: "nested_var_reference_used", Value: true})
30+
}
31+
32+
func TestResolveVariableReferencesNoNestedVarRef(t *testing.T) {
33+
b := &bundle.Bundle{
34+
Config: config.Root{
35+
Bundle: config.Bundle{
36+
Name: "${var.env}",
37+
},
38+
},
39+
}
40+
41+
diags := bundle.Apply(t.Context(), b, ResolveVariableReferencesWithoutResources())
42+
_ = diags
43+
44+
for _, entry := range b.Metrics.BoolValues {
45+
assert.NotEqual(t, "nested_var_reference_used", entry.Key)
46+
}
47+
}
48+
1349
func TestResolveVariableReferencesWithSourceLinkedDeployment(t *testing.T) {
1450
testCases := []struct {
1551
enabled bool
@@ -63,3 +99,52 @@ func TestResolveVariableReferencesWithSourceLinkedDeployment(t *testing.T) {
6399
testCase.assert(t, b)
64100
}
65101
}
102+
103+
func TestResolveVariableReferencesRoundsNoReferences(t *testing.T) {
104+
b := &bundle.Bundle{
105+
Config: config.Root{
106+
Bundle: config.Bundle{
107+
Name: "literal-name",
108+
},
109+
},
110+
}
111+
112+
diags := bundle.Apply(t.Context(), b, ResolveVariableReferencesWithoutResources())
113+
require.NoError(t, diags.Error())
114+
115+
// No references means a single round with no updates, so gt_1 should not be set.
116+
for _, entry := range b.Metrics.BoolValues {
117+
assert.NotEqual(t, "variable_resolution_rounds_gt_1", entry.Key)
118+
assert.NotEqual(t, "variable_resolution_rounds_gt_3", entry.Key)
119+
}
120+
}
121+
122+
func TestResolveVariableReferencesRoundsGt1MultiRound(t *testing.T) {
123+
// Set up a chain: bundle.name -> var.a -> var.b -> literal.
124+
// This requires 2 rounds to fully resolve.
125+
b := &bundle.Bundle{
126+
Config: config.Root{
127+
Bundle: config.Bundle{
128+
Name: "${var.a}",
129+
},
130+
Variables: map[string]*variable.Variable{
131+
"a": {
132+
Value: "${var.b}",
133+
},
134+
"b": {
135+
Value: "final",
136+
},
137+
},
138+
},
139+
}
140+
141+
diags := bundle.Apply(t.Context(), b, ResolveVariableReferencesWithoutResources())
142+
require.NoError(t, diags.Error())
143+
assert.Equal(t, "final", b.Config.Bundle.Name)
144+
145+
assert.Contains(t, b.Metrics.BoolValues, protos.BoolMapEntry{Key: "variable_resolution_rounds_gt_1", Value: true})
146+
// 2 rounds should not trigger gt_3.
147+
for _, entry := range b.Metrics.BoolValues {
148+
assert.NotEqual(t, "variable_resolution_rounds_gt_3", entry.Key)
149+
}
150+
}

bundle/config/mutator/set_variables.go

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -30,30 +30,42 @@ func getDefaultVariableFilePath(target string) string {
3030
return ".databricks/bundle/" + target + "/variable-overrides.json"
3131
}
3232

33-
func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, name string, fileDefault dyn.Value) (dyn.Value, error) {
34-
// case: variable already has value initialized, so skip
33+
// variableOverrideSource tracks which override method resolved a variable's value.
34+
type variableOverrideSource int
35+
36+
const (
37+
variableOverrideSourceCLI variableOverrideSource = iota // --var flag (value already set)
38+
variableOverrideSourceEnvVar // BUNDLE_VAR_* environment variable
39+
variableOverrideSourceFile // variable-overrides.json
40+
variableOverrideSourceDefault // default value from config
41+
variableOverrideSourceNone // no value resolved (lookup or error)
42+
)
43+
44+
func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable, name string, fileDefault dyn.Value) (dyn.Value, variableOverrideSource, error) {
45+
// case: variable already has value initialized, so skip.
46+
// This happens when the value is set via the --var CLI flag.
3547
if variable.HasValue() {
36-
return v, nil
48+
return v, variableOverrideSourceCLI, nil
3749
}
3850

3951
// case: read and set variable value from process environment
4052
envVarName := bundleVarPrefix + name
4153
if val, ok := env.Lookup(ctx, envVarName); ok {
4254
if variable.IsComplex() {
43-
return dyn.InvalidValue, fmt.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
55+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`setting via environment variables (%s) is not supported for complex variable %s`, envVarName, name)
4456
}
4557

4658
v, err := dyn.Set(v, "value", dyn.V(val))
4759
if err != nil {
48-
return dyn.InvalidValue, fmt.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
60+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`failed to assign value "%s" to variable %s from environment variable %s with error: %v`, val, name, envVarName, err)
4961
}
50-
return v, nil
62+
return v, variableOverrideSourceEnvVar, nil
5163
}
5264

5365
// case: Defined a variable for named lookup for a resource
5466
// It will be resolved later in ResolveResourceReferences mutator
5567
if variable.Lookup != nil {
56-
return v, nil
68+
return v, variableOverrideSourceNone, nil
5769
}
5870

5971
// case: Set the variable to the default value from the variable file
@@ -62,36 +74,36 @@ func setVariable(ctx context.Context, v dyn.Value, variable *variable.Variable,
6274
hasComplexValue := fileDefault.Kind() == dyn.KindMap || fileDefault.Kind() == dyn.KindSequence
6375

6476
if hasComplexType && !hasComplexValue {
65-
return dyn.InvalidValue, fmt.Errorf(`variable %s is of type complex, but the value in the variable file is not a complex type`, name)
77+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`variable %s is of type complex, but the value in the variable file is not a complex type`, name)
6678
}
6779
if !hasComplexType && hasComplexValue {
68-
return dyn.InvalidValue, fmt.Errorf(`variable %s is not of type complex, but the value in the variable file is a complex type`, name)
80+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`variable %s is not of type complex, but the value in the variable file is a complex type`, name)
6981
}
7082

7183
v, err := dyn.Set(v, "value", fileDefault)
7284
if err != nil {
73-
return dyn.InvalidValue, fmt.Errorf(`failed to assign default value from variable file to variable %s with error: %v`, name, err)
85+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`failed to assign default value from variable file to variable %s with error: %v`, name, err)
7486
}
7587

76-
return v, nil
88+
return v, variableOverrideSourceFile, nil
7789
}
7890

7991
// case: Set the variable to its default value
8092
if variable.HasDefault() {
8193
vDefault, err := dyn.Get(v, "default")
8294
if err != nil {
83-
return dyn.InvalidValue, fmt.Errorf(`failed to get default value from config "%s" for variable %s with error: %v`, variable.Default, name, err)
95+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`failed to get default value from config "%s" for variable %s with error: %v`, variable.Default, name, err)
8496
}
8597

8698
v, err := dyn.Set(v, "value", vDefault)
8799
if err != nil {
88-
return dyn.InvalidValue, fmt.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, variable.Default, name, err)
100+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`failed to assign default value from config "%s" to variable %s with error: %v`, variable.Default, name, err)
89101
}
90-
return v, nil
102+
return v, variableOverrideSourceDefault, nil
91103
}
92104

93105
// We should have had a value to set for the variable at this point.
94-
return dyn.InvalidValue, fmt.Errorf(`no value assigned to required variable %s. Variables are usually assigned in databricks.yml, and they can be overridden using "--var", the %s environment variable, or %s`, name, bundleVarPrefix+name, getDefaultVariableFilePath("<target>"))
106+
return dyn.InvalidValue, variableOverrideSourceNone, fmt.Errorf(`no value assigned to required variable %s. Variables are usually assigned in databricks.yml, and they can be overridden using "--var", the %s environment variable, or %s`, name, bundleVarPrefix+name, getDefaultVariableFilePath("<target>"))
95107
}
96108

97109
func readVariablesFromFile(b *bundle.Bundle) (dyn.Value, diag.Diagnostics) {
@@ -128,6 +140,11 @@ func (m *setVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos
128140
if diags.HasError() {
129141
return diags
130142
}
143+
144+
envVarUsed := false
145+
fileUsed := false
146+
cliUsed := false
147+
131148
err := b.Config.Mutate(func(v dyn.Value) (dyn.Value, error) {
132149
return dyn.Map(v, "variables", dyn.Foreach(func(p dyn.Path, variable dyn.Value) (dyn.Value, error) {
133150
name := p[1].Key()
@@ -137,9 +154,28 @@ func (m *setVariables) Apply(ctx context.Context, b *bundle.Bundle) diag.Diagnos
137154
}
138155

139156
fileDefault, _ := dyn.Get(defaults, name)
140-
return setVariable(ctx, variable, v, name, fileDefault)
157+
result, source, err := setVariable(ctx, variable, v, name, fileDefault)
158+
switch source {
159+
case variableOverrideSourceCLI:
160+
cliUsed = true
161+
case variableOverrideSourceEnvVar:
162+
envVarUsed = true
163+
case variableOverrideSourceFile:
164+
fileUsed = true
165+
}
166+
return result, err
141167
}))
142168
})
143169

170+
if envVarUsed {
171+
b.Metrics.SetBoolValue("variable_override_env_var_used", true)
172+
}
173+
if fileUsed {
174+
b.Metrics.SetBoolValue("variable_override_file_used", true)
175+
}
176+
if cliUsed {
177+
b.Metrics.SetBoolValue("variable_override_cli_flag_used", true)
178+
}
179+
144180
return diags.Extend(diag.FromErr(err))
145181
}

0 commit comments

Comments
 (0)