Skip to content

Commit c17380c

Browse files
Track the execution times of bundle mutators (#3097)
## Changes This PR modifies the `bundle.Apply` method to allow us to track execution times for all mutators. It also separately tracks the execution time of the three phases: Initialise, Build and Deploy. ## Why Tracking the execution times will give us insight into the time sinks where users spend the most time and thus areas we should invest in improving to give users a faster experience when using the CLI. Note: This information can also be extrapolated (to some degree) to other commands. For example, we could look at how long `phases.Inititalize` takes to understand the latencies involved in the bundle validate command. ## Tests Update existing acceptance test.
1 parent 4ce207e commit c17380c

File tree

7 files changed

+154
-13
lines changed

7 files changed

+154
-13
lines changed

acceptance/bundle/telemetry/deploy/output.txt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,7 @@ Updating deployment state...
66
Deployment complete!
77

88
>>> cat out.requests.txt
9+
10+
=== Assert that there are atleast 5 mutators for which the execution time is recorded
11+
>>> cat telemetry.json
12+
true

acceptance/bundle/telemetry/deploy/script

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
trace $CLI bundle deploy
22

3-
trace cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson' > out.telemetry.txt
3+
trace cat out.requests.txt | jq 'select(has("path") and .path == "/telemetry-ext") | .body.protoLogs[] | fromjson' > telemetry.json
4+
5+
# Note that the number 5 here is arbitrary. In practice I saw 16 mutators be recorded in acceptance test runs so 5 seems
6+
# like a reasonably safe number to assert that mutator execution times are being recorded.
7+
title "Assert that there are atleast 5 mutators for which the execution time is recorded"
8+
trace cat telemetry.json | jq ' .entry.databricks_cli_log.bundle_deploy_event.experimental.bundle_mutator_execution_time_ms | length > 5'
9+
10+
# bundle_mutator_execution_time_ms can have variable number of entries depending upon the runtime of the mutators. Thus we omit it from
11+
# being asserted here.
12+
cat telemetry.json | jq 'del(.entry.databricks_cli_log.bundle_deploy_event.experimental.bundle_mutator_execution_time_ms)' > out.telemetry.txt
413

514
cmd_exec_id=$(extract_command_exec_id.py)
615
deployment_id=$(cat .databricks/bundle/default/deployment.json | jq -r .id)
@@ -10,3 +19,4 @@ update_file.py out.telemetry.txt "11111111-2222-3333-4444-555555555555" '[BUNDL
1019
update_file.py out.telemetry.txt $deployment_id '[DEPLOYMENT_ID]'
1120

1221
rm out.requests.txt
22+
rm telemetry.json

bundle/bundle.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@ type Metrics struct {
5252
BoolValues []protos.BoolMapEntry
5353
PythonAddedResourcesCount int64
5454
PythonUpdatedResourcesCount int64
55+
ExecutionTimes []protos.IntMapEntry
5556
}
5657

5758
func (m *Metrics) AddBoolValue(key string, value bool) {

bundle/mutator.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,12 @@ package bundle
22

33
import (
44
"context"
5+
"reflect"
6+
"time"
57

68
"github.com/databricks/cli/libs/diag"
79
"github.com/databricks/cli/libs/log"
10+
"github.com/databricks/cli/libs/telemetry/protos"
811
)
912

1013
// Mutator is the interface type that mutates a bundle's configuration or internal state.
@@ -17,7 +20,55 @@ type Mutator interface {
1720
Apply(context.Context, *Bundle) diag.Diagnostics
1821
}
1922

23+
// safeMutatorName returns the package name and type name of the underlying mutator type
24+
// in the format "package_name.(type_name)".
25+
//
26+
// We cannot rely on the .Name() method for the name here because it can contain user
27+
// input which would then leak into the telemetry. E.g. [SetDefault] or [loader.ProcessInclude]
28+
func safeMutatorName(m Mutator) string {
29+
t := reflect.TypeOf(m)
30+
31+
// Handle pointer types by getting the element type
32+
if t.Kind() == reflect.Ptr {
33+
t = t.Elem()
34+
}
35+
36+
// Get the package path and type name
37+
pkgPath := t.PkgPath()
38+
typeName := t.Name()
39+
40+
// Extract just the package name from the full package path
41+
// e.g., "github.com/databricks/cli/bundle/config/mutator" -> "mutator"
42+
packageName := pkgPath
43+
if lastSlash := len(pkgPath) - 1; lastSlash >= 0 {
44+
for i := lastSlash; i >= 0; i-- {
45+
if pkgPath[i] == '/' {
46+
packageName = pkgPath[i+1:]
47+
break
48+
}
49+
}
50+
}
51+
52+
return packageName + ".(" + typeName + ")"
53+
}
54+
2055
func Apply(ctx context.Context, b *Bundle, m Mutator) diag.Diagnostics {
56+
// Track the execution time of the mutator.
57+
t0 := time.Now()
58+
defer func() {
59+
duration := time.Since(t0).Milliseconds()
60+
61+
// Don't track the mutator if it takes less than 1ms to execute.
62+
if duration == 0 {
63+
return
64+
}
65+
66+
b.Metrics.ExecutionTimes = append(b.Metrics.ExecutionTimes, protos.IntMapEntry{
67+
Key: safeMutatorName(m),
68+
Value: duration,
69+
})
70+
}()
71+
2172
ctx = log.NewContext(ctx, log.GetLogger(ctx).With("mutator", m.Name()))
2273

2374
log.Debugf(ctx, "Apply")

bundle/mutator_test.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"testing"
66

77
"github.com/databricks/cli/libs/diag"
8+
"github.com/databricks/cli/libs/dyn"
89
"github.com/stretchr/testify/assert"
910
)
1011

@@ -43,3 +44,34 @@ func TestMutator(t *testing.T) {
4344
assert.Equal(t, 1, nested[0].applyCalled)
4445
assert.Equal(t, 1, nested[1].applyCalled)
4546
}
47+
48+
func TestSafeMutatorName(t *testing.T) {
49+
tests := []struct {
50+
name string
51+
mutator Mutator
52+
expected string
53+
}{
54+
{
55+
name: "funcMutator",
56+
mutator: funcMutator{fn: nil},
57+
expected: "bundle.(funcMutator)",
58+
},
59+
{
60+
name: "setDefault mutator",
61+
mutator: SetDefaultMutator(dyn.NewPattern(dyn.Key("test")), "key", "value"),
62+
expected: "bundle.(setDefault)",
63+
},
64+
{
65+
name: "funcMutator as pointer",
66+
mutator: &funcMutator{fn: nil},
67+
expected: "bundle.(funcMutator)",
68+
},
69+
}
70+
71+
for _, tt := range tests {
72+
t.Run(tt.name, func(t *testing.T) {
73+
result := safeMutatorName(tt.mutator)
74+
assert.Equal(t, tt.expected, result, "mutatorName should return correct package.type format")
75+
})
76+
}
77+
}

bundle/phases/telemetry.go

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package phases
33
import (
44
"context"
55
"slices"
6+
"sort"
67

78
"github.com/databricks/cli/bundle"
89
"github.com/databricks/cli/bundle/config"
@@ -13,6 +14,25 @@ import (
1314
"github.com/databricks/cli/libs/telemetry/protos"
1415
)
1516

17+
func getExecutionTimes(b *bundle.Bundle) []protos.IntMapEntry {
18+
executionTimes := b.Metrics.ExecutionTimes
19+
20+
// Sort the execution times in descending order.
21+
sort.Slice(executionTimes, func(i, j int) bool {
22+
return executionTimes[i].Value > executionTimes[j].Value
23+
})
24+
25+
// Keep only the top 250 execution times. This keeps the telemetry event
26+
// reasonable in size. This should be unnecessary in most cases but is
27+
// done out of caution since the number of mutators depends upon user input.
28+
// Eg: every pattern in `includes:` triggers a new mutator.
29+
if len(executionTimes) > 250 {
30+
executionTimes = executionTimes[:250]
31+
}
32+
33+
return executionTimes
34+
}
35+
1636
func logDeployTelemetry(ctx context.Context, b *bundle.Bundle) {
1737
resourcesCount := int64(0)
1838
_, err := dyn.MapByPattern(b.Config.Value(), dyn.NewPattern(dyn.Key("resources"), dyn.AnyKey(), dyn.AnyKey()), func(p dyn.Path, v dyn.Value) (dyn.Value, error) {
@@ -149,18 +169,19 @@ func logDeployTelemetry(ctx context.Context, b *bundle.Bundle) {
149169
ResourceDashboardIDs: dashboardIds,
150170

151171
Experimental: &protos.BundleDeployExperimental{
152-
BundleMode: mode,
153-
ConfigurationFileCount: b.Metrics.ConfigurationFileCount,
154-
TargetCount: b.Metrics.TargetCount,
155-
WorkspaceArtifactPathType: artifactPathType,
156-
BoolValues: b.Metrics.BoolValues,
157-
PythonAddedResourcesCount: b.Metrics.PythonAddedResourcesCount,
158-
PythonUpdatedResourcesCount: b.Metrics.PythonUpdatedResourcesCount,
159-
PythonResourceLoadersCount: int64(len(experimentalConfig.Python.Resources)),
160-
PythonResourceMutatorsCount: int64(len(experimentalConfig.Python.Mutators)),
161-
VariableCount: int64(variableCount),
162-
ComplexVariableCount: complexVariableCount,
163-
LookupVariableCount: lookupVariableCount,
172+
BundleMode: mode,
173+
ConfigurationFileCount: b.Metrics.ConfigurationFileCount,
174+
TargetCount: b.Metrics.TargetCount,
175+
WorkspaceArtifactPathType: artifactPathType,
176+
BoolValues: b.Metrics.BoolValues,
177+
PythonAddedResourcesCount: b.Metrics.PythonAddedResourcesCount,
178+
PythonUpdatedResourcesCount: b.Metrics.PythonUpdatedResourcesCount,
179+
PythonResourceLoadersCount: int64(len(experimentalConfig.Python.Resources)),
180+
PythonResourceMutatorsCount: int64(len(experimentalConfig.Python.Mutators)),
181+
VariableCount: int64(variableCount),
182+
ComplexVariableCount: complexVariableCount,
183+
LookupVariableCount: lookupVariableCount,
184+
BundleMutatorExecutionTimeMs: getExecutionTimes(b),
164185
},
165186
},
166187
})

cmd/bundle/deploy.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"io"
7+
"time"
78

89
"github.com/databricks/cli/bundle"
910
"github.com/databricks/cli/bundle/config/validate"
@@ -13,6 +14,7 @@ import (
1314
"github.com/databricks/cli/cmd/root"
1415
"github.com/databricks/cli/libs/diag"
1516
"github.com/databricks/cli/libs/sync"
17+
"github.com/databricks/cli/libs/telemetry/protos"
1618
"github.com/spf13/cobra"
1719
)
1820

@@ -70,18 +72,38 @@ func newDeployCommand() *cobra.Command {
7072
}
7173
}
7274

75+
t0 := time.Now()
7376
diags = diags.Extend(phases.Initialize(ctx, b))
77+
b.Metrics.ExecutionTimes = append(b.Metrics.ExecutionTimes, protos.IntMapEntry{
78+
Key: "phases.Initialize",
79+
Value: time.Since(t0).Milliseconds(),
80+
})
7481

7582
if !diags.HasError() {
83+
t1 := time.Now()
7684
diags = diags.Extend(bundle.Apply(ctx, b, validate.FastValidate()))
85+
b.Metrics.ExecutionTimes = append(b.Metrics.ExecutionTimes, protos.IntMapEntry{
86+
Key: "validate.FastValidate",
87+
Value: time.Since(t1).Milliseconds(),
88+
})
7789
}
7890

7991
if !diags.HasError() {
92+
t2 := time.Now()
8093
diags = diags.Extend(phases.Build(ctx, b))
94+
b.Metrics.ExecutionTimes = append(b.Metrics.ExecutionTimes, protos.IntMapEntry{
95+
Key: "phases.Build",
96+
Value: time.Since(t2).Milliseconds(),
97+
})
8198
}
8299

83100
if !diags.HasError() {
101+
t3 := time.Now()
84102
diags = diags.Extend(phases.Deploy(ctx, b, outputHandler))
103+
b.Metrics.ExecutionTimes = append(b.Metrics.ExecutionTimes, protos.IntMapEntry{
104+
Key: "phases.Deploy",
105+
Value: time.Since(t3).Milliseconds(),
106+
})
85107
}
86108
}
87109

0 commit comments

Comments
 (0)