Add synchronous logger for telemetry #2432

shreyas-goenka · 2025-03-05T13:54:43Z

Changes

This PR adds a synchronous telemetry logger for the CLI with a max timeout of 3 seconds. Due to the 3-second timeout configuration, this is only meant to be used in long-running commands.

This is a short-term solution. Eventually, we'd like to transition to a daemon process to upload the telemetry logs to amortise the costs of configuring authentication and maintaining a warm pool of HTTP connections, as well as a better UX for the end user.

Note that users can set the DATABRICKS_CLI_DISABLE_TELEMETRY environment variable to disable telemetry collection.

Why

To collect telemetry, which was previously inaccessible to us, and answer questions like which templates customers like to use and which DABs features would be safe to deprecate.

Tests

Unit and acceptance tests.

Also manually verified that the telemetry upload works:

(artifact-playground) ➜  cli git:(sync-logger) cli selftest send-telemetry --debug
15:58:20 Info: start pid=40386 version=0.0.0-dev+a2825ca89a23 args="cli, selftest, send-telemetry, --debug"
15:58:20 Debug: Loading DEFAULT profile from /Users/shreyas.goenka/.databrickscfg pid=40386 sdk=true
15:58:20 Info: completed execution pid=40386 exit_code=0
15:58:21 Debug: POST /telemetry-ext
> {
>   "items": null,
>   "protoLogs": [
>     "{\"frontend_log_event_id\":\"82d29b3a-d5ff-48f3-8a21-dae6e08d2999\",\"entry\":{\"databricks_cli_log\":{\"... (232 more bytes)",
>     "{\"frontend_log_event_id\":\"d6be8220-7db8-45d9-97d6-4c09c25e2664\",\"entry\":{\"databricks_cli_log\":{\"... (232 more bytes)"
>   ],
>   "uploadTime": 1741186700967
> }
< HTTP/2.0 200 OK
< {
<   "errors": null,
<   "numProtoSuccess": 2,
<   "numSuccess": 0
< } pid=40386 sdk=true

cmd/root/root.go

pietern · 2025-03-06T08:10:47Z

cmd/root/user_agent_command_exec_id.go

@@ -7,8 +7,10 @@ import (
 	"github.com/google/uuid"
 )

+var cmdExecId = uuid.New().String()
+


This is a global now. It should be initialized once per command and can be set on the context.

Done in #2439. Will update this PR once the other one is merged.

libs/telemetry/logger.go

pietern · 2025-03-06T08:21:30Z

libs/telemetry/logger.go

+		protoLogs[i] = string(b)
+	}
+
+	apiClient, err := client.New(cfg)


A new client means we cannot reuse existing connections. Can this function take a workspace client directly?

I don't think so. Anytime we need to make a bespoke API call, we rely on creating a new client.

I can look into whether we can reuse the existing connections as a followup optimization.

libs/telemetry/logger.go

acceptance/config_test.go

denik · 2025-03-06T11:54:33Z

acceptance/telemetry/timeout/test.toml

+    "numProtoSuccess": 2
+}
+'''
+DelaySeconds = 5


Could we add env var DATABRICKS_CLI_TELEMETRY_TIMEOUT and set it to 0.01 in tests so that we don't have to wait 3 seconds here?

The tests run in parallel anyway so it does not add latency.

Not configuring the timeout makes the test more representative of the real behaviour, but I'm fine with making it configurable with an environment variable if you still recommend doing so.

libs/telemetry/logger.go

denik · 2025-03-06T12:04:05Z

cmd/root/root.go

+		ExitCode:        int64(exitCode),
+	})
+
+	err := telemetry.Upload(ctx, ConfigUsed(ctx))


Why not hide SetExecutionContext/HasLogs/env var checking inside telemetry packages? So the code would look like this:

telemetry.Upload(ctx, ... ExecutionContext{...}) // updates logs with execution context, checks env var if upload is needed

Moved SetExecutionContext inside and the env var code inside. We cannot move HasLogs because not every command would configure a config in the context, and for those cases ConfigUsed(ctx)) would panic.

I can work on a followup PR that consolidates ConfigUsed and other clients in the libs/command library. Ref: #2439

Once #2440 is merged, we can get rid of HasLogs as well.

## Changes This PR also starts storing the DBR versions in the context. Go patch file used: ``` @@ var x expression @@ -dbr.MockRuntime(x, true) +dbr.MockRuntime(x, dbr.Environment{IsDbr: true, Version: "15.4"}) @@ var x expression @@ -dbr.MockRuntime(x, false) +dbr.MockRuntime(x, dbr.Environment{}) ``` ref: https://github.com/uber-go/gopatch ## Why This localised all DBR version accesses to `libs/dbr`. Relevant comment: #2432 (comment) ## Tests Exiting tests are modified.

## Changes This PR moves `ConfigUsed` from the root package to `libs/command`. ## Why Having the ConfigUsed function in the root package is a problem because that means we cannot use that function from outside the `root` package since doing so often leads to an import cycle (because `root` imports everything implicitly). Moving it to a separate package that consolidates the read interaction and solves the import cycle issue. Example where this would have simplified code: #2432 (comment) I'd like to send PRs to do the same for the workspace client and account client as well. I'll wait however until this one is merged incase people have concerns about the approach here. ## Tests Existing tests.

Add synchronous logger for telemetry

f6592d9

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 13:54 — with GitHub Actions Inactive

add sync logger

b3a479c

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 14:21 — with GitHub Actions Inactive

add acceptance tests

a2825ca

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 14:57 — with GitHub Actions Inactive

-

a652b44

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:00 — with GitHub Actions Inactive

shreyas-goenka added 2 commits March 5, 2025 16:09

add test for timeout

972741d

-

f227862

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:09 — with GitHub Actions Inactive

debug logs everywhere

d80a5ec

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:21 — with GitHub Actions Inactive

some cleanup

9774046

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:27 — with GitHub Actions Inactive

shreyas-goenka added 2 commits March 5, 2025 16:29

Merge remote-tracking branch 'origin' into sync-logger

709e8c5

serialize execution time

6da7466

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:30 — with GitHub Actions Inactive

N more bytes

69b38ec

shreyas-goenka temporarily deployed to test-trigger-is March 5, 2025 15:36 — with GitHub Actions Inactive

shreyas-goenka marked this pull request as ready for review March 5, 2025 15:41

shreyas-goenka requested review from pietern, andrewnester and denik as code owners March 5, 2025 15:41

shreyas-goenka mentioned this pull request Mar 5, 2025

Remove omitempty tag for exit code and execution time in telemetry #2433

Merged

pietern reviewed Mar 6, 2025

View reviewed changes

shreyas-goenka mentioned this pull request Mar 6, 2025

Store DBR version in context #2438

Merged

use env var

7b61d7f

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 09:57 — with GitHub Actions Inactive

remove 503 detection

0ac9a2a

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 10:05 — with GitHub Actions Inactive

make upload and wait times a constant

3811d24

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 10:36 — with GitHub Actions Inactive

shreyas-goenka mentioned this pull request Mar 6, 2025

Add library to store cmd-exec-id in context #2439

Open

denik reviewed Mar 6, 2025

View reviewed changes

acceptance/config_test.go Outdated Show resolved Hide resolved

denik reviewed Mar 6, 2025

View reviewed changes

support float delays

0404dc4

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 12:46 — with GitHub Actions Inactive

shreyas-goenka added 4 commits March 6, 2025 14:14

-

5b058e0

update acceptance tests

aa4e231

merge

d1ec239

read runtime version from context

aec6b06

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 13:33 — with GitHub Actions Inactive

fix tests

a5bbd1f

shreyas-goenka temporarily deployed to test-trigger-is March 6, 2025 13:39 — with GitHub Actions Inactive

shreyas-goenka requested review from pietern and denik March 6, 2025 13:44

shreyas-goenka mentioned this pull request Mar 6, 2025

Move ConfigUsed to libs/command #2440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add synchronous logger for telemetry #2432

Add synchronous logger for telemetry #2432

shreyas-goenka commented Mar 5, 2025 •

edited

Loading

pietern Mar 6, 2025

shreyas-goenka Mar 6, 2025

pietern Mar 6, 2025

shreyas-goenka Mar 6, 2025

denik Mar 6, 2025

shreyas-goenka Mar 6, 2025 •

edited

Loading

denik Mar 6, 2025

shreyas-goenka Mar 6, 2025

shreyas-goenka Mar 6, 2025

shreyas-goenka Mar 6, 2025

Add synchronous logger for telemetry #2432

Are you sure you want to change the base?

Add synchronous logger for telemetry #2432

Conversation

shreyas-goenka commented Mar 5, 2025 • edited Loading

Changes

Why

Tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shreyas-goenka commented Mar 5, 2025 •

edited

Loading

shreyas-goenka Mar 6, 2025 •

edited

Loading