Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add async logger for telemetry #2278

Open
wants to merge 61 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
ab10720
squash all commits
shreyas-goenka Jan 20, 2025
3964d8d
[WIP] In process telemetry logger
shreyas-goenka Jan 22, 2025
01d63dd
Merge remote-tracking branch 'origin' into implement-async-logger
shreyas-goenka Jan 23, 2025
155fe7b
-
shreyas-goenka Jan 29, 2025
acd64fa
merge
shreyas-goenka Jan 29, 2025
ee3568c
-'
shreyas-goenka Jan 29, 2025
8f8463f
-
shreyas-goenka Jan 29, 2025
f092e21
add back worker input
shreyas-goenka Jan 29, 2025
5d75c3f
Merge remote-tracking branch 'origin' into implement-async-logger
shreyas-goenka Feb 2, 2025
427c755
major cleanup
shreyas-goenka Feb 3, 2025
b83e576
-
shreyas-goenka Feb 3, 2025
f09a780
fix panic
shreyas-goenka Feb 3, 2025
90148d8
replace os
shreyas-goenka Feb 3, 2025
259a21a
-
shreyas-goenka Feb 3, 2025
e4a1f42
-
shreyas-goenka Feb 3, 2025
d7bf1dc
add test for upload
shreyas-goenka Feb 3, 2025
382efe4
-
shreyas-goenka Feb 3, 2025
c412eb7
-
shreyas-goenka Feb 3, 2025
da0cf95
-
shreyas-goenka Feb 3, 2025
8801587
pass test
shreyas-goenka Feb 3, 2025
5385faf
fix test
shreyas-goenka Feb 3, 2025
c1a3225
fx test
shreyas-goenka Feb 3, 2025
4f97900
-
shreyas-goenka Feb 3, 2025
a8b366e
-
shreyas-goenka Feb 3, 2025
5c2205a
-
shreyas-goenka Feb 3, 2025
dc0ab30
-
shreyas-goenka Feb 3, 2025
2cd25e3
-
shreyas-goenka Feb 3, 2025
2cbc39f
-
shreyas-goenka Feb 3, 2025
f3e7594
fx test
shreyas-goenka Feb 3, 2025
403f612
address comments
shreyas-goenka Feb 3, 2025
0423b09
add filtering for auth
shreyas-goenka Feb 3, 2025
981dbf7
add bash script for waiting
shreyas-goenka Feb 4, 2025
918af62
remove eventually files
shreyas-goenka Feb 4, 2025
39ff290
pass test
shreyas-goenka Feb 4, 2025
33ff865
cleaner output
shreyas-goenka Feb 4, 2025
17698a5
-
shreyas-goenka Feb 4, 2025
5b6ffd5
-
shreyas-goenka Feb 4, 2025
963022a
-
shreyas-goenka Feb 4, 2025
9e2a689
-
shreyas-goenka Feb 4, 2025
0abba86
=
shreyas-goenka Feb 4, 2025
407e9e0
-
shreyas-goenka Feb 4, 2025
414a94d
-
shreyas-goenka Feb 4, 2025
d5e03f0
-
shreyas-goenka Feb 4, 2025
8c90ad0
clean
shreyas-goenka Feb 4, 2025
1bb4537
Merge remote-tracking branch 'origin' into async-logger-clean
shreyas-goenka Feb 5, 2025
c9ebc82
add error test case'
shreyas-goenka Feb 5, 2025
fd6b129
make test generic
shreyas-goenka Feb 5, 2025
73fac82
remove pid file
shreyas-goenka Feb 5, 2025
5e2e03a
fix test
shreyas-goenka Feb 5, 2025
0253039
-
shreyas-goenka Feb 5, 2025
369faff
return 501
shreyas-goenka Feb 5, 2025
e43a0a0
skip end to end integration tests
shreyas-goenka Feb 6, 2025
f88db77
-
shreyas-goenka Feb 10, 2025
a6e8e92
Merge remote-tracking branch 'origin' into async-logger-clean
shreyas-goenka Feb 10, 2025
23b42e9
Merge remote-tracking branch 'origin' into async-logger-clean
shreyas-goenka Feb 10, 2025
9d65761
pretty print
shreyas-goenka Feb 10, 2025
c8ac08c
consolidate test.toml
shreyas-goenka Feb 10, 2025
58bf931
comment
shreyas-goenka Feb 10, 2025
79fad7a
pass test
shreyas-goenka Feb 10, 2025
4cdcbd6
Merge remote-tracking branch 'origin' into async-logger-clean
shreyas-goenka Feb 11, 2025
f98369d
-
shreyas-goenka Feb 11, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions acceptance/acceptance_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/databricks/cli/libs/testdiff"
"github.com/databricks/cli/libs/testserver"
"github.com/databricks/databricks-sdk-go"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -156,6 +157,7 @@ func testAccept(t *testing.T, InprocessMode bool, singleTest string) int {
testdiff.PrepareReplacementsWorkspaceClient(t, &repls, workspaceClient)
testdiff.PrepareReplacementsUUID(t, &repls)
testdiff.PrepareReplacementsDevVersion(t, &repls)
testdiff.PrepareReplaceOS(t, &repls)

testDirs := getTests(t)
require.NotEmpty(t, testDirs)
Expand Down Expand Up @@ -281,6 +283,21 @@ func runTest(t *testing.T, dir, coverDir string, repls testdiff.ReplacementsCont
cmd.Dir = tmpDir
err = cmd.Run()

// Wait for the files to appear before starting assertion on the output.
// This is useful for concurrency control when the CLI spawns subprocesses.
missingFiles := config.EventuallyFiles
assert.Eventually(t, func() bool {
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
for _, file := range missingFiles {
_, err := os.Stat(filepath.Join(tmpDir, file))
if err == nil {
missingFiles = slices.DeleteFunc(missingFiles, func(n string) bool {
return n == file
})
}
}
return len(missingFiles) == 0
}, 10*time.Second, 100*time.Millisecond, "Files did not appear: %v", missingFiles)

// Write the requests made to the server to a output file if the test is
// configured to record requests.
if config.RecordRequests {
Expand Down
4 changes: 4 additions & 0 deletions acceptance/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ type TestConfig struct {
// Record the requests made to the server and write them as output to
// out.requests.txt
RecordRequests bool

// Wait for these files to appear before starting assertion on the output.
// This is useful for concurrency control when tests spawn subprocesses.
EventuallyFiles []string
}

type ServerStub struct {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error: telemetry endpoint not found
2 changes: 2 additions & 0 deletions acceptance/libs/telemetry/dummy-without-server/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

>>> $CLI telemetry dummy
5 changes: 5 additions & 0 deletions acceptance/libs/telemetry/dummy-without-server/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export DATABRICKS_TELEMETRY_UPLOAD_LOGS_FILE=./out.upload.txt
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved

# This test ensures that the main CLI command does not error even if
# telemetry upload fails.
trace $CLI telemetry dummy
1 change: 1 addition & 0 deletions acceptance/libs/telemetry/dummy/out.requests.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"method":"POST","path":"/telemetry-ext","body":{"uploadTime":"UNIX_TIME_MILLIS","items":[],"protoLogs":["{\"frontend_log_event_id\":\"[UUID]\",\"entry\":{\"databricks_cli_log\":{\"execution_context\":{\"cmd_exec_id\":\"[UUID]\",\"version\":\"$DEV_VERSION\",\"command\":\"telemetry_dummy\",\"operating_system\":\"[OS]\",\"execution_time_ms\":0,\"exit_code\":0},\"cli_test_event\":{\"name\":\"VALUE1\"}}}}","{\"frontend_log_event_id\":\"[UUID]\",\"entry\":{\"databricks_cli_log\":{\"execution_context\":{\"cmd_exec_id\":\"[UUID]\",\"version\":\"$DEV_VERSION\",\"command\":\"telemetry_dummy\",\"operating_system\":\"[OS]\",\"execution_time_ms\":0,\"exit_code\":0},\"cli_test_event\":{\"name\":\"VALUE2\"}}}}"]}}
shreyas-goenka marked this conversation as resolved.
Show resolved Hide resolved
3 changes: 3 additions & 0 deletions acceptance/libs/telemetry/dummy/out.upload.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Successfully uploaded telemetry logs
Response:
{"errors":null,"numProtoSuccess":2}
2 changes: 2 additions & 0 deletions acceptance/libs/telemetry/dummy/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@

>>> $CLI telemetry dummy
3 changes: 3 additions & 0 deletions acceptance/libs/telemetry/dummy/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export DATABRICKS_TELEMETRY_UPLOAD_LOGS_FILE=./out.upload.txt

trace $CLI telemetry dummy
16 changes: 16 additions & 0 deletions acceptance/libs/telemetry/dummy/test.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
RecordRequests = true
EventuallyFiles = [
"out.upload.txt"
]

[[Server]]
Pattern = "POST /telemetry-ext"
Response.Body = '''
{
"numProtoSuccess": 2
}
'''

[[Repls]]
Old = '17\d{11}'
New = '"UNIX_TIME_MILLIS"'
1 change: 1 addition & 0 deletions acceptance/libs/telemetry/upload/out.requests.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"method":"POST","path":"/telemetry-ext","body":{"uploadTime":1738560432192,"items":[],"protoLogs":["{\"frontend_log_event_id\":\"[UUID]\",\"entry\":{\"databricks_cli_log\":{\"cli_test_event\":{\"name\":\"VALUE1\"}}}}","{\"frontend_log_event_id\":\"[UUID]\",\"entry\":{\"databricks_cli_log\":{\"cli_test_event\":{\"name\":\"VALUE2\"}}}}"]}}
5 changes: 5 additions & 0 deletions acceptance/libs/telemetry/upload/output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@

>>> $CLI telemetry upload
Successfully uploaded telemetry logs
Response:
{"errors":null,"numProtoSuccess":2}
3 changes: 3 additions & 0 deletions acceptance/libs/telemetry/upload/script
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# This command / test cannot be run in inprocess / debug mode. This is because
# it does not go through the [root.Execute] function.
trace $CLI telemetry upload < stdin
24 changes: 24 additions & 0 deletions acceptance/libs/telemetry/upload/stdin
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
{
"logs": [
{
"frontend_log_event_id": "BB79BB52-96F6-42C5-9E44-E63EEA84888D",
"entry": {
"databricks_cli_log": {
"cli_test_event": {
"name": "VALUE1"
}
}
}
},
{
"frontend_log_event_id": "A7F597B0-66D1-462D-824C-C5C706F232E8",
"entry": {
"databricks_cli_log": {
"cli_test_event": {
"name": "VALUE2"
}
}
}
}
]
}
9 changes: 9 additions & 0 deletions acceptance/libs/telemetry/upload/test.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
RecordRequests = true

[[Server]]
Pattern = "POST /telemetry-ext"
Response.Body = '''
{
"numProtoSuccess": 2
}
'''
3 changes: 2 additions & 1 deletion cmd/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/databricks/cli/cmd/labs"
"github.com/databricks/cli/cmd/root"
"github.com/databricks/cli/cmd/sync"
"github.com/databricks/cli/cmd/telemetry"
"github.com/databricks/cli/cmd/version"
"github.com/databricks/cli/cmd/workspace"
"github.com/spf13/cobra"
Expand Down Expand Up @@ -74,6 +75,6 @@ func New(ctx context.Context) *cobra.Command {
cli.AddCommand(labs.New(ctx))
cli.AddCommand(sync.New())
cli.AddCommand(version.New())

cli.AddCommand(telemetry.New())
return cli
}
82 changes: 78 additions & 4 deletions cmd/root/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,24 @@ package root

import (
"context"
"encoding/json"
"errors"
"fmt"
"log/slog"
"os"
"os/exec"
"runtime"
"strings"
"time"

"github.com/databricks/cli/internal/build"
"github.com/databricks/cli/libs/auth"
"github.com/databricks/cli/libs/cmdio"
"github.com/databricks/cli/libs/dbr"
"github.com/databricks/cli/libs/env"
"github.com/databricks/cli/libs/log"
"github.com/databricks/cli/libs/telemetry"
"github.com/databricks/cli/libs/telemetry/protos"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -73,9 +81,6 @@ func New(ctx context.Context) *cobra.Command {
// get the context back
ctx = cmd.Context()

// Detect if the CLI is running on DBR and store this on the context.
ctx = dbr.DetectRuntime(ctx)

// Configure our user agent with the command that's about to be executed.
ctx = withCommandInUserAgent(ctx, cmd)
ctx = withCommandExecIdInUserAgent(ctx)
Expand All @@ -97,7 +102,9 @@ func flagErrorFunc(c *cobra.Command, err error) error {
// Execute adds all child commands to the root command and sets flags appropriately.
// This is called by main.main(). It only needs to happen once to the rootCmd.
func Execute(ctx context.Context, cmd *cobra.Command) error {
// TODO: deferred panic recovery
ctx = telemetry.WithNewLogger(ctx)
ctx = dbr.DetectRuntime(ctx)
start := time.Now()

// Run the command
cmd, err := cmd.ExecuteContextC(ctx)
Expand All @@ -121,5 +128,72 @@ func Execute(ctx context.Context, cmd *cobra.Command) error {
}
}

end := time.Now()
exitCode := 0
if err != nil {
exitCode = 1
}

uploadTelemetry(cmd.Context(), commandString(cmd), start, end, exitCode)
return err
}

func uploadTelemetry(ctx context.Context, cmdStr string, start, end time.Time, exitCode int) {
// Nothing to upload.
if !telemetry.HasLogs(ctx) {
return
}

telemetry.SetExecutionContext(ctx, protos.ExecutionContext{
CmdExecID: cmdExecId,
Version: build.GetInfo().Version,
Command: cmdStr,
OperatingSystem: runtime.GOOS,
DbrVersion: env.Get(ctx, dbr.EnvVarName),
ExecutionTimeMs: end.Sub(start).Milliseconds(),
ExitCode: int64(exitCode),
})

logs := telemetry.GetLogs(ctx)

in := telemetry.UploadConfig{
Logs: logs,
}

execPath, err := os.Executable()
if err != nil {
log.Debugf(ctx, "failed to get executable path: %s", err)
}
telemetryCmd := exec.Command(execPath, "telemetry", "upload")
telemetryCmd.Env = os.Environ()
for k, v := range auth.Env(ConfigUsed(ctx)) {
telemetryCmd.Env = append(telemetryCmd.Env, fmt.Sprintf("%s=%s", k, v))
}

b, err := json.Marshal(in)
if err != nil {
log.Debugf(ctx, "failed to marshal telemetry logs: %s", err)
return
}

stdin, err := telemetryCmd.StdinPipe()
if err != nil {
log.Debugf(ctx, "failed to create stdin pipe for telemetry worker: %s", err)
}

err = telemetryCmd.Start()
if err != nil {
log.Debugf(ctx, "failed to start telemetry worker: %s", err)
return
}

_, err = stdin.Write(b)
if err != nil {
log.Debugf(ctx, "failed to write to telemetry worker: %s", err)
}

err = stdin.Close()
if err != nil {
log.Debugf(ctx, "failed to close stdin for telemetry worker: %s", err)
}
}
4 changes: 3 additions & 1 deletion cmd/root/user_agent_command_exec_id.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import (
"github.com/google/uuid"
)

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

func withCommandExecIdInUserAgent(ctx context.Context) context.Context {
// A UUID that will allow us to correlate multiple API requests made by
// the same CLI invocation.
return useragent.InContext(ctx, "cmd-exec-id", uuid.New().String())
return useragent.InContext(ctx, "cmd-exec-id", cmdExecId)
}
31 changes: 31 additions & 0 deletions cmd/telemetry/dummy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package telemetry

import (
"github.com/databricks/cli/cmd/root"
"github.com/databricks/cli/libs/telemetry"
"github.com/databricks/cli/libs/telemetry/protos"
"github.com/spf13/cobra"
)

func newDummyCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "dummy",
Short: "log dummy telemetry events",
Long: "Fire a test telemetry event against the configured Databricks workspace.",
Hidden: true,
PreRunE: root.MustWorkspaceClient,
}

cmd.RunE = func(cmd *cobra.Command, args []string) error {
for _, v := range []string{"VALUE1", "VALUE2"} {
telemetry.Log(cmd.Context(), protos.DatabricksCliLog{
CliTestEvent: &protos.CliTestEvent{
Name: protos.DummyCliEnum(v),
},
})
}
return nil
}

return cmd
}
16 changes: 16 additions & 0 deletions cmd/telemetry/telemetry.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package telemetry

import (
"github.com/spf13/cobra"
)

func New() *cobra.Command {
cmd := &cobra.Command{
Use: "telemetry",
Short: "",
Hidden: true,
}

cmd.AddCommand(newDummyCommand())
return cmd
}
4 changes: 3 additions & 1 deletion libs/dbr/detect.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
// Dereference [os.Stat] to allow mocking in tests.
var statFunc = os.Stat

const EnvVarName = "DATABRICKS_RUNTIME_VERSION"

// detect returns true if the current process is running on a Databricks Runtime.
// Its return value is meant to be cached in the context.
func detect(ctx context.Context) bool {
Expand All @@ -21,7 +23,7 @@ func detect(ctx context.Context) bool {
}

// Databricks Runtime always has the DATABRICKS_RUNTIME_VERSION environment variable set.
if value, ok := env.Lookup(ctx, "DATABRICKS_RUNTIME_VERSION"); !ok || value == "" {
if value, ok := env.Lookup(ctx, EnvVarName); !ok || value == "" {
return false
}

Expand Down
25 changes: 25 additions & 0 deletions libs/telemetry/context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package telemetry

import (
"context"
"fmt"
)

// Private type to store the telemetry logger in the context
type telemetryLogger int

// Key to store the telemetry logger in the context
var telemetryLoggerKey telemetryLogger

func WithNewLogger(ctx context.Context) context.Context {
return context.WithValue(ctx, telemetryLoggerKey, &logger{})
}

func fromContext(ctx context.Context) *logger {
v := ctx.Value(telemetryLoggerKey)
if v == nil {
panic(fmt.Errorf("telemetry logger not found in the context"))

Check failure on line 21 in libs/telemetry/context.go

View workflow job for this annotation

GitHub Actions / lint

fmt.Errorf can be replaced with errors.New (perfsprint)
}

return v.(*logger)
}
Loading
Loading