diff --git a/experimental/ssh/internal/client/client_internal_test.go b/experimental/ssh/internal/client/client_internal_test.go index f71f38d0898..216c4f85822 100644 --- a/experimental/ssh/internal/client/client_internal_test.go +++ b/experimental/ssh/internal/client/client_internal_test.go @@ -1,6 +1,7 @@ package client import ( + "encoding/json" "errors" "strings" "testing" @@ -482,3 +483,16 @@ func TestBuildSshTunnelEvent(t *testing.T) { }) } } + +// A failed first attempt is the case the telemetry exists to measure, so assert +// the outcome fields reach the payload as an explicit false rather than being +// dropped as zero values. +func TestBuildSshTunnelEventReportsFailure(t *testing.T) { + got := buildSshTunnelEvent(ClientOptions{ClusterID: "abc-123"}, false, false, 0) + + assert.False(t, got.IsSuccess) + + b, err := json.Marshal(got) + require.NoError(t, err) + assert.Contains(t, string(b), `"is_success":false`) +} diff --git a/libs/telemetry/protos/ssh_tunnel.go b/libs/telemetry/protos/ssh_tunnel.go index c359002be1f..c15fca9af35 100644 --- a/libs/telemetry/protos/ssh_tunnel.go +++ b/libs/telemetry/protos/ssh_tunnel.go @@ -19,6 +19,13 @@ const ( // SshTunnelEvent is emitted when a user establishes an SSH tunnel connection // via the Databricks CLI. +// +// Every bool below is populated on every event, so none of them carry +// omitempty: a genuine false must stay distinguishable from an older CLI that +// did not report the field. Events sent before CLI v1.14.0 omitted false +// entirely and land in the table as NULL, so queries spanning that cutover must +// count NULL as false (e.g. a failed connection is `is_success IS NULL` for +// pre-v1.14.0 rows and `is_success = false` after). type SshTunnelEvent struct { // Type of compute: dedicated cluster or serverless. ComputeType SshTunnelComputeType `json:"compute_type,omitempty"` @@ -33,24 +40,24 @@ type SshTunnelEvent struct { ClientMode SshTunnelClientMode `json:"client_mode,omitempty"` // Whether this is a reconnection to an existing session. - IsReconnect bool `json:"is_reconnect,omitempty"` + IsReconnect bool `json:"is_reconnect"` // Whether the cluster was auto-started by the CLI. - AutoStartCluster bool `json:"auto_start_cluster,omitempty"` + AutoStartCluster bool `json:"auto_start_cluster"` // Whether a custom base environment was set via --base-environment. // Only the presence is recorded: the flag value can be an env.yaml path // or display name carrying PII, so the value itself is not logged. - HasBaseEnvironment bool `json:"has_base_environment,omitempty"` + HasBaseEnvironment bool `json:"has_base_environment"` // Time in milliseconds spent starting the SSH server. // Zero if server was already running. ServerStartTimeMs int64 `json:"server_start_time_ms"` // Whether the connection was successful. - IsSuccess bool `json:"is_success,omitempty"` + IsSuccess bool `json:"is_success"` // Whether a serverless usage policy was set via --usage-policy-id. // Only the presence is recorded, not the policy ID itself. - HasUsagePolicy bool `json:"has_usage_policy,omitempty"` + HasUsagePolicy bool `json:"has_usage_policy"` } diff --git a/libs/telemetry/protos/ssh_tunnel_test.go b/libs/telemetry/protos/ssh_tunnel_test.go new file mode 100644 index 00000000000..d4ac094f716 --- /dev/null +++ b/libs/telemetry/protos/ssh_tunnel_test.go @@ -0,0 +1,45 @@ +package protos + +import ( + "encoding/json" + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A failed connection is the zero value of every bool, so omitempty would drop +// the fields entirely and make the failure indistinguishable from an unreported +// one. This pins the wire payload for that case. +func TestSshTunnelEventEncodesFailureExplicitly(t *testing.T) { + b, err := json.Marshal(SshTunnelEvent{}) + require.NoError(t, err) + + var got map[string]any + require.NoError(t, json.Unmarshal(b, &got)) + + for _, field := range []string{ + "is_success", + "is_reconnect", + "auto_start_cluster", + "has_base_environment", + "has_usage_policy", + } { + assert.Equal(t, false, got[field], "%s must be sent as false, not omitted", field) + } +} + +// Guards fields added later: a bool that can legitimately be false must not +// carry omitempty, or its false case arrives as NULL and cannot be counted. +func TestSshTunnelEventBoolFieldsOmitOmitempty(t *testing.T) { + typ := reflect.TypeFor[SshTunnelEvent]() + for field := range typ.Fields() { + if field.Type.Kind() != reflect.Bool { + continue + } + tag := field.Tag.Get("json") + assert.NotContains(t, tag, "omitempty", + "%s has omitempty; a false value would be indistinguishable from not reported", field.Name) + } +}