Repository navigation
Conversation
|
Commit: Regression test reportSelected 2 changed tests (limit=20, 0 not selected) 2 tests: regression=2 unreleased=0 coverage=0 failing=0
TestAccept/cmd/aitools/telemetry/apps-init-tip: branch ✅ | main (634e6bf) ❌ | latest (v1.26.0) ❌main (634e6bf): latest (v1.26.0): TestAccept/cmd/aitools/telemetry/hint: branch ✅ | main (634e6bf) ❌ | latest (v1.26.0) ❌main (634e6bf): latest (v1.26.0): |
✅ Integration test reportCommit: 2c02b39
Top 15 slowest tests (at least 2 minutes):
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 2ca5f55. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 3 open design questions.
This PR logs an aitools_hint_event each time the CLI shows an install hint, the apps init tip or the apps init prompt. It also bounds the whole telemetry upload, including config resolution and token fetch, by the upload timeout. The event's wire shape matches the Universe proto, the timeout claim holds for SDK v0.197.0, and CI is green. Nothing is blocking. The main open points are: an in-flight U2M token refresh can be abandoned at exit; a few test and acceptance-script convention issues; and three questions on scope, exit latency and APPS_INIT_* data quality.
Questions
- The Upload timeout change in libs/telemetry/logger.go (newAPIClientWithin, newAPIClient, authenticate and the three timeout tests) changes exit behavior for every telemetry upload: bundle deploy, the ssh agent shim, air and environments. The hint event doesn't need it, and the hint_test.go agent-env cleanup is also separate. AGENTS.md asks for one change per PR. Would you split these into their own PRs? If they stay here, would you add a .nextchanges/cli fragment for the fix that keeps the CLI from hanging at exit when auth stalls?
- Commands that print the install hint (for example current-user me or jobs list) used to log nothing and skip the telemetry upload. They now wait at exit for POST /telemetry-ext, up to the 3s default. That happens once per Claude session per hour, or on every command when CLAUDE_CODE_SESSION_ID is unset. Is that cost acceptable for this metric? Or would a cheaper signal work, such as a shorter timeout for hint-only uploads or a User-Agent segment?
- The apps init guard (HasDatabricksSkillsInstalled) checks only skills dirs, so APPS_INIT_TIP and APPS_INIT_PROMPT are also logged for Claude Code users who already have the plugin. They are also logged for humans with an agent config dir, and calling_agent is not sent to tell them apart. Should the guard be fixed first, should APPS_INIT_* be held back for now, or should this be accepted and documented for whoever builds the dashboards?
Comments outside the changed lines
- [nit]
libs/aitools/agents/hint.go:53: "Best-effort: it writes only to stderr" is no longer true:MaybeHintnow also buffers a telemetry event, which makes a command that resolved auth upload at exit. The same goes for "(best-effort, stderr only)" atcmd/root/root.go:93. Please mention the telemetry event in both.
| // authenticate sets the auth headers on r, giving up when r's context, which | ||
| // carries the upload deadline, is done. It authenticates a clone so that an | ||
| // abandoned goroutine never writes to r. | ||
| func authenticate(r *http.Request, cfg *config.Config) error { |
There was a problem hiding this comment.
[nice to have] authenticate abandons cfg.Authenticate at the upload deadline, including a databricks-cli (U2M) token refresh that is already in flight. Suppose the access token expired during a long command, and the refresh takes longer than what is left of the 3s budget. The process then exits before PersistentAuth stores the new token. If the server already rotated the refresh token, the next command fails to refresh until databricks auth login; #7468 documents the same hazard for its background auth. Before this PR, Upload waited and the refreshed token was saved.
Separately, the safety note at line 209 ("Callers run Upload just before the process exits or execs") does not hold when the agent shim's execProcess fails. cmd/root then reads the same cfg in EnrichAuthError and calls Upload again, while the abandoned goroutine may still be writing to it.
Please state the refresh risk in Merge Danger, or decide whether abandoning an in-flight refresh is acceptable here. Also narrow the line-209 comment to what callers actually guarantee.
There was a problem hiding this comment.
Stated the refresh risk in Merge Danger: an in-flight U2M (databricks-cli) refresh that outlasts the rest of the budget is abandoned, and if the server already rotated the refresh token, the next command needs databricks auth login. Abandoning it is in line with what the timeout is for (no hang at exit), so I kept the behavior.
The comment above within (it replaces the detached block) now says only what callers guarantee: the goroutine keeps running until the SDK call returns or the process ends, cmd/root uploads as the last step before exit, and the ssh agent shim uploads right before exec. If the exec fails, cmd/root reads the same config and uploads again while that goroutine may still be running.
In 0326a2e.
There was a problem hiding this comment.
The exec-failure race is now fixed in 29d7bbc. The logger counts the goroutines of uploads it stopped waiting for. While one is still running, a later Upload on the same logger returns a previous telemetry upload is still running and doesn't touch the config. That second Upload is the one cmd/root runs when the shim's execProcess fails. TestTelemetryUploadSkipsWhileEarlierUploadRuns covers this.
The other read on that path doesn't race. EnrichAuthError returns before reading cfg unless the error is a 401/403 APIError, and an exec failure isn't one.
The refresh risk stays as documented. Merge Danger now names both budgets: 3s, or 1s for a hint-only upload.
| } | ||
| cmdio.LogString(ctx, hintText) | ||
| LogHintShown(ctx, protos.AitoolsHintKindInstallText) | ||
| recordSessionHint(ctx) |
There was a problem hiding this comment.
[nice to have] recordSessionHint marks the session as hinted even when this command's event can never upload. Take a Claude session whose first non-aitools command resolves no auth, e.g. databricks version or auth profiles:
- the hint prints;
- the event is dropped at exit, because there is no
ConfigUsed; - for the next hour, later commands in that session print no hint and log nothing.
So the session contributes zero hint events even though the agent saw the hint. That skews "hints shown vs installs" low. Please document this session-level loss next to the "uploads only when the command resolved auth" note in protos/README.md, or decide whether the metric can accept it before dashboards depend on it.
There was a problem hiding this comment.
Documented it in libs/telemetry/protos/README.md, right after the "uploads only when the command resolved auth" sentence: because the hint prints once per Claude Code session per hour, a session whose hinted command resolves no auth (e.g. databricks version) logs no install hint event for that hour. Merge Danger says the same. In 0326a2e.
| unset CLAUDE_CONFIG_DIR | ||
|
|
||
| title "hint printed on an authenticated command: one hint-shown event is uploaded\n" | ||
| trace $CLI current-user me > /dev/null |
There was a problem hiding this comment.
[nice to have] This case relies on current-user me printing the hint. Open #7452 adds current-user me, along with auth, configure, help and completion, to the hint-exempt commands. Whichever PR lands second breaks this golden: no hint is printed and no event is uploaded (lines 9 and 18). A command that #7452 keeps hinting and the testserver serves, such as jobs list, avoids the conflict.
| // before the process exits or execs, so it does not outlive them. | ||
|
|
||
| // newAPIClientWithin runs newAPIClient, giving up when ctx is done. | ||
| func newAPIClientWithin(ctx context.Context, cfg *config.Config) (*client.DatabricksClient, error) { |
There was a problem hiding this comment.
[nice to have] newAPIClientWithin and authenticate repeat the same goroutine/select/abandon logic. newAPIClient re-implements client.New by hand only to swap the AuthVisitor, so later SDK changes to client.New won't reach this path. Instead, run the client construction and the attempt loop in one goroutine, and select on its result against ctx.Done(). That bounds the same steps with one mechanism: config resolution, the token fetch on every attempt, and the requests. It also keeps client.New, and drops the request clone and the detached 10-line comment block above these functions.
There was a problem hiding this comment.
I tried the single goroutine (client.New plus the attempt loop, selected against ctx.Done()), and it broke acceptance/telemetry/timeout. At the deadline, Upload returns on ctx.Done() before the request goroutine reports its own context deadline exceeded. So the --debug lines for the request, non-retriable error and Attempt 1 failed due to a timeout are lost or printed after telemetry upload failed, depending on scheduling, and the logged error loses the Post "<url>/telemetry-ext" detail. Requests honor ctx, so the patch keeps them on the caller's goroutine and moves only the calls that ignore ctx off it.
What I changed instead:
- The duplicated goroutine/select/abandon code is now one helper,
within(ctx, f). Both client construction andauthenticateuse it. - The detached comment block became that helper's doc comment.
I kept newAPIClient, because the per-request token fetch (including the one on a retry) can only be bounded through the AuthVisitor. client.New has no option to set it. I kept the request clone too: it stops an abandoned cfg.Authenticate from writing to the headers of a request that is already being sent. In 0326a2e.
| # recorded requests so the next call sees only the next command's upload. | ||
| print_telemetry() { | ||
| if [ -f "$OUT_REQUESTS" ]; then | ||
| jq -c 'select(.path == "/telemetry-ext") | .body.protoLogs[] | fromjson |
There was a problem hiding this comment.
[nice to have] print_telemetry filters $OUT_REQUESTS with a hand-written jq 'select(.path == "/telemetry-ext") ...' and then rm. .agents/rules/testing.md says: "Filter recorded requests with print_requests.py, never with a hand-written jq 'select(...)' out.requests.txt pipeline — inline, or hidden inside a local print_requests() shell function." Wrapping the helper is allowed, so use jq only for decoding: print_requests.py --oneline //telemetry-ext | jq -c '.body.protoLogs[] | fromjson | ...'. Keep the [ -f "$OUT_REQUESTS" ] guard, because print_requests.py exits with an error when the file is missing (the version case).
There was a problem hiding this comment.
print_telemetry now wraps print_requests.py --oneline //telemetry-ext and uses jq only to decode protoLogs. The [ -f "$OUT_REQUESTS" ] guard stays for the version case, and print_requests.py deletes the file. In 0326a2e.
| assert.Equal(t, []protos.AitoolsHintEvent{{HintKind: protos.AitoolsHintKindInstallText}}, hintEvents(t, ctx)) | ||
| } | ||
|
|
||
| func TestMaybeHint_NotPrintedLogsNothing(t *testing.T) { |
There was a problem hiding this comment.
[nit] Several new tests repeat existing scenarios:
TestMaybeHint_NotPrintedLogsNothingrepeatsHumanCallerSilent,PluginInstalledSilent,AIToolsCommandSilentandNoIONoPanic.TestMaybeHint_ThrottledSessionLogsOncecopiesTestMaybeHint_ThrottledPerSession, including the inlinenewCtx := func()closure that.agents/rules/style-guide-go.mddisallows.- The
RecommendSkillsInstall*tests repeatrecommend_test.go'sNonInteractive,InteractiveDeclineandSkipsWhenSkillsExist.
Wrapping the existing tests' ctx with telemetry.WithNewLogger and adding the event assertion gives the same coverage without a parallel suite. The RecommendSkillsInstall assertions would then live in recommend_test.go, next to recommend.go.
There was a problem hiding this comment.
Removed hint_telemetry_test.go. The event assertions are now in the existing tests, each with its ctx wrapped in telemetry.WithNewLogger:
ClaudeNotInstalledHints:[INSTALL_TEXT].HumanCallerSilent,PluginInstalledSilent,AIToolsCommandSilentandNoIONoPanic: no events.ThrottledPerSession: one event, then none. It keeps its existingnewCtxclosure; I didn't add a new one.recommend_test.go:NonInteractivegets[APPS_INIT_TIP],InteractiveDeclinegets[APPS_INIT_PROMPT],SkipsWhenSkillsExistgets none.
In 0326a2e.
| // BufferedEvents returns the events logged on ctx, or nil when ctx has no | ||
| // logger. Upload does not clear them. Tests in other packages use it to assert | ||
| // what a command logs without uploading. | ||
| func BufferedEvents(ctx context.Context) []protos.DatabricksCliLog { |
There was a problem hiding this comment.
[nit] BufferedEvents is exported from libs/telemetry only for tests in other packages. The repo already checks events from other packages by handling POST /telemetry-ext on a testserver and calling telemetry.Upload (cmd/air/telemetry_test.go, cmd/environments/telemetry_test.go). Using that pattern keeps the buffer private. Likewise, LogHintShown is only called inside package agents and could be unexported.
There was a problem hiding this comment.
LogHintShown is now unexported (logHintShown).
I kept BufferedEvents exported. The next PRs in this stack (#7469 and #7470) use it from cmd/aitools/funnel_test.go to check the install/update/uninstall funnel events command by command. Moving those to a testserver plus Upload would change those PRs. If you'd still rather keep it private, I'll make the change across the stack. In 0326a2e.
| unset CLAUDE_CODE_SESSION_ID | ||
| unset CLAUDE_CONFIG_DIR | ||
| # A Claude Code config dir with no Databricks skills is what makes apps init print its tip. | ||
| mkdir -p "${USERPROFILE:-$HOME}/.claude" |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md says: "Commit static test inputs into the acceptance test directory; do not create them in script at test time." Commit home/.claude/ with a placeholder file, as hint/installed-home does, instead of mkdir -p "${USERPROFILE:-$HOME}/.claude". (sethome sets both variables, so the fallback is redundant.) The same file also sends traced output to > /dev/null, which the rules say should go to LOG.<name>.
There was a problem hiding this comment.
The script now uses a committed apps-init-tip/home/.claude/.gitkeep instead of the mkdir. The apps init stdout goes to LOG.apps-init, not /dev/null. In 0326a2e.
| "home", | ||
| ] | ||
|
|
||
| [[Server]] |
There was a problem hiding this comment.
[nit] This stub exists because libs/testserver/handlers.go:612 always returns numProtoSuccess: 1. With that default, the two-event apps init upload is a partial success and gets retried. .agents/rules/testing.md says to model API behavior in libs/testserver/ rather than in per-test [[Server]] stubs. Having the shared handler return len(protoLogs) would let this file, and the older stubs in acceptance/telemetry, drop the override.
There was a problem hiding this comment.
The shared POST /telemetry-ext handler in libs/testserver/handlers.go now returns numProtoSuccess: len(protoLogs), and this file's [[Server]] stub is gone. The full acceptance suite passes with the new default. I left the older stubs in acceptance/telemetry alone: partial-success needs its stub, and dropping the others is a cleanup outside this PR. In 0326a2e.
| server := testserver.New(t) | ||
| t.Cleanup(server.Close) | ||
|
|
||
| expiry := time.Now().Add(500 * time.Millisecond) |
There was a problem hiding this comment.
[nit] expiry is fixed before Upload starts. If resolving the config and authenticating the first request take more than 500ms, the first attempt already sees an expired token, blocks in the second token fetch, and never reaches the server. count is then 0 and the test fails. That can happen on a loaded CI runner under -race. Setting expiry when the token source first returns a token, and sharing it with the handler via an atomic, removes the dependency on start-up time.
There was a problem hiding this comment.
The token source now sets the expiry when it returns the first token, 500ms after that, and shares it with the handler through an atomic.Pointer[time.Time]. Start-up time no longer matters. In 0326a2e.
2ca5f55 to
0326a2e
Compare
|
Thanks for the review. All 11 inline comments are answered in their threads. Changes in 0326a2e:
I haven't answered the three Questions in the review body yet; I'll follow up on them separately. |
0326a2e to
673bfd5
Compare
673bfd5 to
e1c80b3
Compare
|
Answers to the three Questions, in e1c80b3:
|
e1c80b3 to
71b4c1a
Compare
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 71b4c1a. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 2 open design questions.
This PR logs an aitools_hint_event whenever the CLI shows the Claude Code install hint or the apps init tip or prompt. It also bounds the whole telemetry upload, including config resolution and token fetch, by its timeout, and keeps apps init quiet when an agent has the databricks plugin. The code does what the description says, CI is green, and the telemetry and agents tests pass under -race; nothing is blocking. Still open: accepted apps init prompts lead to installs that are never recorded, the abandoned token refresh from the earlier thread, a few nits, and two questions about scope and exit-time cost.
Questions
- This PR also changes how every telemetry upload is bounded (bundle deploy, the ssh agent shim, air and environments), including abandoning an in-flight U2M token refresh at the deadline. It also changes when
apps initshows its tip. You explained that the hint event is what makes these commands upload, so the changes belong together. Would you be open to landing the upload-bounding change as its own PR, with this one on top, so it can be reviewed and reverted on its own? If you'd rather keep a single PR, could the title mention the upload change so it shows up in history? - Hinted commands that used to skip telemetry now upload at exit and can wait up to 1s.
cmdctx.SetConfigUsedruns before auth succeeds, so a hinted command whose own auth failed also reruns the default credential chain at exit (for example the Azure CLI strategy) until the 1s deadline. Is that cost acceptable for this metric? Or would you skip a hint-only upload when the command's credentials never resolved, for example whencfg.AuthTypeis empty?
Comments outside the changed lines
- [nit]
.github/PULL_REQUEST_TEMPLATE.md:.agents/skills/pr-checklist/SKILL.mdasks PR bodies to use the template's## Changes/## Why/## Testsheadings in order, without new sections such as## Summary. The Evidence block would fit under## Tests. Also, Merge Danger says an in-flight U2M refresh is abandoned when it "outlasts the rest of the 3s budget". Hint-only uploads, which are the uploads this PR adds, get 1s (hintOnlyUploadTimeout), so please mention the 1s case too.
Agreeing with existing threads
- [nice to have] Abandoned in-flight U2M refresh at the upload deadline; race on cfg when the shim's exec fails: #7467 (comment)
| return nil | ||
| } | ||
|
|
||
| logHintShown(ctx, protos.AitoolsHintKindAppsInitPrompt) |
There was a problem hiding this comment.
[nice to have] APPS_INIT_PROMPT is logged before the question, but nothing records the answer or the install it starts. When the user answers yes, installFn (installer.InstallAllSkills) installs the skills without logging an AitoolsInstallEvent; only cmd/aitools logs one. So in telemetry an accepted prompt and a declined one look the same, and this hint kind can't be compared with the installs that follow, which is what the event is for. Either log the outcome of the prompt path (an install event after installFn succeeds, or an accepted field once the proto has one), or note in libs/telemetry/protos/README.md that APPS_INIT_PROMPT conversions are not observable.
There was a problem hiding this comment.
Not in this PR. Recording the answer, or the install the prompt starts, needs a new field or event, and this stack doesn't change the telemetry schema. Recording it can follow separately. For now, Merge Danger says that an accepted APPS_INIT_PROMPT and a declined one look the same.
| if flagsMode { | ||
| if !agents.HasDatabricksSkillsInstalled(ctx) { | ||
| cmdio.LogString(ctx, "Tip: coding agents detected without Databricks skills. Run 'databricks aitools install' to install them.") | ||
| if !agents.HasDatabricksToolingInstalled(ctx) { |
There was a problem hiding this comment.
[nit] apps init now skips its skills tip and its "Install skills now?" prompt when a detected agent has the databricks plugin, which users will notice. .agents/skills/pr-checklist/SKILL.md lists changed command behavior as needing a .nextchanges fragment, but the only fragment added covers the upload timeout. Please add one, e.g. .nextchanges/cli/apps-init-plugin-tip.md: "* apps init no longer suggests installing skills when a detected coding agent has the Databricks plugin. (#7467)"
There was a problem hiding this comment.
Added .nextchanges/cli/apps-init-plugin-tip.md with your wording and the PR link. In 29d7bbc.
| err error | ||
| } | ||
| resc := make(chan result, 1) | ||
| go func() { |
There was a problem hiding this comment.
[nit] The goroutine in within has no recover. Config resolution and cfg.Authenticate used to run on the caller's goroutine, so a panic there (for example in a credentials strategy) was caught by the deferred recover in root.Execute, which prints the "unexpectedly had a fatal error" report. Now it is an unrecovered goroutine panic: a raw runtime crash at exit, after the command itself succeeded. Recovering inside the goroutine and sending the panic back as an error (resc <- result{err: fmt.Errorf("panic: %v", r)}) turns it into an ordinary "telemetry upload failed" log line.
There was a problem hiding this comment.
The goroutine in within now recovers and returns the panic as an error (panic: <value>), so it shows up as an ordinary telemetry upload failed log line. TestWithinReturnsPanicAsError covers this. In 29d7bbc.
| func HasDatabricksToolingInstalled(ctx context.Context) bool { | ||
| for _, a := range DetectInstalled(ctx) { | ||
| if _, ok := a.DatabricksPluginVersion(ctx); ok { | ||
| return true | ||
| } | ||
| } | ||
| return HasDatabricksSkillsInstalled(ctx) |
There was a problem hiding this comment.
[nit] The loop calls DetectInstalled, and then HasDatabricksSkillsInstalled calls it again, so each apps init probes every registry agent's config dir twice. .agents/rules/style-guide-go.md asks to avoid unnecessary loops. Call DetectInstalled once, return true when it is empty, check the plugins, then check the canonical and legacy skill dirs. A small hasSharedSkills(ctx) helper shared with HasDatabricksSkillsInstalled would do it.
There was a problem hiding this comment.
HasDatabricksToolingInstalled now calls DetectInstalled once: it returns true when no agent is detected, then checks the plugins, then the shared skills dirs. The canonical/legacy check moved into hasSharedSkills, which HasDatabricksSkillsInstalled also uses. In 29d7bbc.
| // The event is buffered and uploaded by cmd/root at exit, which needs resolved | ||
| // auth: a hint shown on a command that never resolves auth (e.g. `databricks | ||
| // version`) is not uploaded. | ||
| func logHintShown(ctx context.Context, kind protos.AitoolsHintKind) { |
There was a problem hiding this comment.
[nit] .agents/rules/testing.md says each source file should have a corresponding test file, and hint_telemetry.go has none. Its only function is already covered by the event assertions in hint_test.go and recommend_test.go. Moving logHintShown into hint.go keeps that coverage without adding an untested file or a duplicate test suite.
There was a problem hiding this comment.
Moved logHintShown into hint.go and removed hint_telemetry.go. The event assertions in hint_test.go and recommend_test.go still cover it. In 29d7bbc.
71b4c1a to
29d7bbc
Compare
anton-107
changed the title
|
Thanks for the second pass. Changes in 29d7bbc; the inline threads have the details. Questions
Outside the diff
Other nits
|
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
🤖 Automated review from simonfaltum's review agent (pr-review skill) at 29d7bbc. Each finding was verified against the code; simonfaltum may follow up.
Not approving yet: 3 open design questions.
This PR logs an install-hint telemetry event for the Claude Code hint and the apps init tip and prompt. It bounds the whole telemetry upload, including config resolution and token fetch, by its timeout, and keeps apps init quiet when an agent has the databricks plugin. CI is green, the telemetry and agents tests pass under -race, and nothing is blocking. The main issue is that the new cfg.AuthType == \"\" gate gets auth success wrong both ways. It reruns credentials for databricks auth login profiles whose auth failed, and it drops hint events from commands run inside bundle directories. There are also a few nits and three open questions: scope, whether the event is needed beyond the existing User-Agent signal, and how broad the apps init check should be.
Questions
- You've explained why you'd keep the hint event and the upload-bounding change in one PR, with logger.go revertible on its own. logger.go now also carries hint-specific logic (onlyHintEvents, hintOnlyUploadTimeout and the AuthType gate), so reverting the bounding change alone would also touch the hint path. AGENTS.md asks for one change per PR. Would you land the upload-bounding change (within/newAPIClient/authenticate and their tests) as its own PR under this one? Or do you see the coupling as a reason to keep them together?
- Every command already sends agent/claude-code plus aitools-agent/<plugin|skills|none> in its User-Agent, which installer/useragent.go describes as giving a per-calling-agent install rate. What does the new event let you measure that this signal can't? The event has no session key to join a hint to a later install, and it adds an exit-time upload of up to 1s on hinted commands (on every command when CLAUDE_CODE_SESSION_ID is unset). Would a User-Agent marker for 'hint shown' be enough, or is the separate event needed for a specific dashboard?
HasDatabricksToolingInstalledtreats tooling as installed when any one detected agent has the databricks plugin. With Claude Code (plugin installed) and Cursor (no skills) both detected,apps initnow shows neither the tip nor the install prompt, althoughInstallAllSkillswould set Cursor up. Before, only shared skills, which every agent uses, counted. Is hiding the recommendation for the other agents intended? Or should the check require every detected agent to have the plugin or the shared skills?
Agreeing with existing threads
- [nice to have] In-flight U2M token refresh is abandoned at the upload deadline: #7467 (comment)
- [nit] PR body doesn't use the repo's Changes/Why/Tests template: #7467 (review)
| // Hints are shown before the command authenticates, and the SDK sets | ||
| // AuthType only once authentication succeeds. When the command's own auth | ||
| // never succeeded, uploading would run the credential chain again at exit | ||
| // for events that would not otherwise be uploaded at all. | ||
| cfg := cmdctx.ConfigUsed(ctx) | ||
| if onlyHintEvents(l.logs) && cfg.AuthType == "" { |
There was a problem hiding this comment.
[nice to have] cfg.AuthType == "" doesn't track whether the command's auth succeeded, and it errs in both directions. (1) The SDK fills AuthType from the profile or DATABRICKS_AUTH_TYPE during EnsureResolved, before authenticating, and every databricks auth login profile has auth_type = databricks-cli (cmd/auth/login.go:559). So when a hinted jobs list fails auth (revoked refresh token, VPN down), the exit-time upload still reruns the U2M strategy, up to three times within the 1s budget. I reproduced this with a databricks-cli profile and no cached token. TestTelemetryUploadHintOnlyNeedsResolvedAuth marks the config as resolved with a user-set AuthType: "pat", so it can't catch this. (2) Inside a bundle directory, MustWorkspaceClient stores a fresh b.Config.Workspace.Config(ctx) as ConfigUsed (cmd/root/auth.go:398), but authenticates a different config built by Workspace.Client, because Workspace.Config returns a new *config.Config on every call. The stored one never gets an AuthType, so hint events from jobs list and similar in bundle projects are dropped even though auth succeeded. Could you gate on a signal that is set only once the client the command uses has authenticated, for example a flag recorded in cmd/root after Authenticate returns nil, with the bundle path covered too? Please add regression cases for a preset auth type with failing credentials and for a bundle directory, and update the comment, protos/README.md and the PR description to match.
There was a problem hiding this comment.
You're right on both counts, so I removed the gate in 2c02b39.
A signal that tracks real auth success isn't contained in this PR. The SDK has no accessor that says a config has authenticated. Bundle commands such as bundle validate and bundle deploy don't authenticate in cmd/root either: configureBundle only builds the client (its comment notes there is no network I/O), and authentication happens on the bundle's own client when the bundle phases make their first request. A flag recorded after Authenticate in MustWorkspaceClient/MustAccountClient would therefore drop hint events from every bundle command. Covering them would mean hooking the bundle client paths, which is outside this change.
With the gate gone:
- A hinted command whose own auth failed tries the upload again at exit, and the hint-only timeout bounds that to 1s.
- Hint events from
jobs listand similar commands in a bundle directory upload again.
Both directions are tested:
TestTelemetryUploadHintOnlyWithFailingAuthIsBoundeduses a presetAuthType: "databricks-cli"whose token source keeps failing: no request is sent andUploadgives up within 1s.TestTelemetryUploadHintOnlyWithUnresolvedConfiguses a config that never authenticated, as in a bundle directory: the event uploads. It fails with the old gate.
The code comment and the PR description are updated. protos/README.md never described the gate, so it needed no change.
| @@ -24,11 +24,33 @@ const ( | |||
| // Returns true if no agents are detected (nothing to recommend) or if skills exist in | |||
| // ~/.databricks/aitools/skills/ or the legacy ~/.databricks/agent-skills/. | |||
| func HasDatabricksSkillsInstalled(ctx context.Context) bool { | |||
There was a problem hiding this comment.
[nit] With apps init and RecommendSkillsInstall switched to HasDatabricksToolingInstalled, HasDatabricksSkillsInstalled is only called from skills_test.go. CI's deadcode -test won't flag a func reachable only from tests, which is the case AGENTS.md:122 describes. Please remove it and fold its test cases into TestHasDatabricksToolingInstalled (or point them at hasSharedSkills). If a later PR in the stack needs it, mark it //deadcode:allow <reason>.
There was a problem hiding this comment.
Removed HasDatabricksSkillsInstalled in 2c02b39. No caller is left in this PR or in the PRs above it. Its test cases now target HasDatabricksToolingInstalled; their agents have no plugin reader, so they still exercise the skills-only path.
| return true | ||
| } | ||
| for _, a := range installed { | ||
| if _, ok := a.DatabricksPluginVersion(ctx); ok { |
There was a problem hiding this comment.
[nit] DatabricksPluginVersion returns ok for a plugin record in any scope or project. With only {"scope":"project","projectPath":"/work/other"} in installed_plugins.json, databricks apps init in /work/new skips the tip and the install prompt, although the plugin won't load for the new app. For a check about the project being created, databricksPluginActiveIn(ctx, cwd) (already used for the User-Agent) matches better. Or, if the install hint's "any trace of an install" semantics are intended here too, a short comment saying so would help.
There was a problem hiding this comment.
The check now uses databricksPluginActiveIn(ctx, cwd), with the working directory resolved the same way the User-Agent code resolves it. A plugin recorded only for another project no longer counts. It also requires the plugin for every detected agent (see Q3). TestHasDatabricksToolingInstalled has a "plugin for another project only" case, which fails with DatabricksPluginVersion. In 2c02b39.
|
|
||
| // hasSharedSkills reports whether Databricks skills are in the canonical | ||
| // skills dir or the legacy one. | ||
| func hasSharedSkills(ctx context.Context) bool { |
There was a problem hiding this comment.
[nit] hasSharedSkills is the same canonical-or-legacy check that claudeToolingInstalled does inline (hint.go:117-122). Having claudeToolingInstalled call hasSharedSkills keeps the install hint and the apps init tip from drifting apart if a skills location is added.
There was a problem hiding this comment.
claudeToolingInstalled now calls hasSharedSkills, so the install hint and the apps init check read the same skills locations. In 2c02b39.
The CLI logs a new `aitools_hint_event` each time it shows a recommendation to install Databricks AI tools, so hints shown can be compared with installs that follow. It also bounds the whole telemetry upload, including client construction and authentication, by the upload timeout.
```diff
MaybeHint(ctx, cmd) # non-aitools command run by Claude Code without the tools
cmdio.LogString(hintText)
+ logHintShown(INSTALL_TEXT)
runCreate (apps init)
flags mode, skills missing
- cmdio.LogString("Tip: ...")
+ PrintSkillsTip # prints the same tip
+ logHintShown(APPS_INIT_TIP)
otherwise: RecommendSkillsInstall, skills missing
no prompt possible
- cmdio.LogString("Tip: ...")
+ PrintSkillsTip # APPS_INIT_TIP
+ logHintShown(APPS_INIT_PROMPT) # before asking, so a declined prompt counts
AskYesOrNo(...)
```
Wire shape (one entry per hint shown):
```json
{"aitools_hint_event": {"hint_kind": "INSTALL_TEXT"}}
```
- The event carries only the hint kind. The command it was shown on is the entry's `execution_context.command`. The calling agent is not a field: the SDK already sends `agent/<name>` in the User-Agent of the telemetry upload.
- `AitoolsHintKind` also declares `CLAUDE_CODE_NATIVE` and `AGENT_INSTALL_TEXT`, which nothing emits yet.
- One `apps init` run can log two events (`INSTALL_TEXT` from the root hint and `APPS_INIT_TIP`), so count per `hint_kind`.
- The `apps init` tip and prompt now also stay silent when every detected agent has the databricks plugin for the working directory in its own plugin manifest, not only when the shared skills are installed (`HasDatabricksToolingInstalled`).
Upload timeout (`libs/telemetry/logger.go`). The SDK resolves the config (including host metadata discovery) and fetches tokens with `context.Background()`, so before this change the upload timeout bounded only the HTTP requests. A slow token endpoint or host could hold the command's exit far past the timeout.
```diff
Upload(ctx)
- client.New(cfg) # unbounded: config resolution, host metadata
ctx = WithTimeout(uploadTimeout) # 3s, DATABRICKS_CLI_TELEMETRY_TIMEOUT
+ within(ctx, newAPIClient(cfg)) # client construction gives up at the deadline
+ AuthVisitor = authenticate # every request, SDK retries included:
+ within(ctx, cfg.Authenticate) # gives up at the deadline
for 3 attempts: attempt(ctx, ...) # requests stay on the caller's goroutine
```
An upload whose events are all hint events uses a 1s timeout instead of the 3s default, since most commands that show a hint would otherwise not upload at all. `DATABRICKS_CLI_TELEMETRY_TIMEOUT`, when set, applies to every upload.
On timeout the goroutine started by `within` is abandoned; a panic in it is returned as an error. `cmd/root` runs `Upload` as the last step before the process exits; the ssh agent shim runs it just before exec, and while an abandoned goroutine still runs, a second `Upload` on the same logger is skipped.
Supporting changes:
```text
libs/telemetry/logger.go BufferedEvents(ctx): read buffered events from tests in other packages
libs/testserver/handlers.go POST /telemetry-ext accepts every log in the request
acceptance/cmd/aitools/telemetry/ print_telemetry helper + `hint` and `apps-init-tip` cases
libs/aitools/agents/hint_test.go uses testutil.SetAgentEnv instead of its own agent env list
```
Changelog fragments: `.nextchanges/cli/telemetry-upload-timeout.md`, `.nextchanges/cli/apps-init-plugin-tip.md`.
Co-authored-by: Isaac <no-reply@databricks.com>
29d7bbc to
2c02b39
Compare
|
Thanks for the third pass. Changes in 2c02b39; the inline threads have the details. Questions
PR body template: I've kept the current format; the 16 PRs in this stack use one consistent format. Happy to switch if you feel strongly. |
Summary
The CLI logs a new
aitools_hint_eventeach time it shows a recommendation to install Databricks AI tools, so hints shown can be compared with installs that follow. It also bounds the whole telemetry upload, including client construction and authentication, by the upload timeout.Wire shape (one entry per hint shown):
{"aitools_hint_event": {"hint_kind": "INSTALL_TEXT"}}execution_context.command. The calling agent is not a field: the SDK already sendsagent/<name>in the User-Agent of the telemetry upload.AitoolsHintKindalso declaresCLAUDE_CODE_NATIVEandAGENT_INSTALL_TEXT, which nothing emits yet.apps initrun can log two events (INSTALL_TEXTfrom the root hint andAPPS_INIT_TIP), so count perhint_kind.apps inittip and prompt now also stay silent when every detected agent has the databricks plugin in its own plugin manifest, for every project or for the working directory, not only when the shared skills are installed (HasDatabricksToolingInstalled, reusingdatabricksPluginActiveIn). A detected agent without either still gets the recommendation.Upload timeout (
libs/telemetry/logger.go). The SDK resolves the config (including host metadata discovery) and fetches tokens withcontext.Background(), so before this change the upload timeout bounded only the HTTP requests. A slow token endpoint or host could hold the command's exit far past the timeout.An upload whose events are all hint events uses a 1s timeout instead of the 3s default (
uploadTimeout), since most commands that show a hint would otherwise not upload at all.DATABRICKS_CLI_TELEMETRY_TIMEOUT, when set, applies to every upload. A hinted command whose own auth failed still tries the upload, so it runs the credential chain again at exit, bounded by the same 1s.On timeout the goroutine started by
withinis abandoned; a panic in it is returned as an error and logged like any upload failure.cmd/rootrunsUploadas the last step before the process exits; the ssh agent shim runs it just before exec. If that exec fails,cmd/rootuploads again on the same logger; while an abandoned goroutine of the first upload still runs, that secondUploadis skipped, so it does not use the config concurrently.Supporting changes:
Evidence
Upload timeout, with
Uploadtemporarily switched back toclient.New(cfg)(the token source, host metadata resolver and token refresh block until the test ends), then at this PR's head:Hint-only timeout, at this PR's head:
The last three fail with their checks removed.
Hint event, at this PR's head (these tests assert the buffered and uploaded events; they were not re-run here with the call sites reverted):
apps initguard:TestRecommendSkillsInstallSkipsWhenPluginInstalledand the plugin case ofapps-init-tipfail with the old skills-only guard and pass at this head.TestHasDatabricksToolingInstalledcovers the plugin for every agent, for one of two agents, for another project only, shared skills, and neither; the one-of-two and other-project cases fail with an any-agent, any-project check.Also passing at this head:
go build ./...,./task test-exp-aitools,go test ./libs/telemetry/... ./libs/aitools/... ./libs/testserver/ ./cmd/apps/... ./cmd/aitools/... ./cmd/air/... ./cmd/environments/... ./experimental/ssh/..., the full acceptance suite (go test ./acceptance),./task lint-q,./task fmt-q,./task checks,./task check-changelog, andgo vetwithGOOS=windowsandGOOS=darwin. In./cmd/root,TestWorkspaceClientOrPromptRejectsPATOnSPOGFromConfigFilefails locally both with and without this change.Merge Danger
Door: two-way
The event is additive and reverting stops sending it. No upload to a real workspace was run here, only the acceptance test server and a marshal test that pins the wire keys.
Blast Radius: telemetry
/telemetry-extis slow or unreachable; commands that also log other events keep the 3s default. Before, most such commands (current-user me,jobs list, ...) logged nothing and skipped the upload. Hint and tip texts are unchanged.telemetry upload failed: ..., info level incmd/root, debug in the shim); before, it waited for the SDK call to finish. Exit codes are unchanged: upload errors were and are only logged. The upload also abandons a U2M (databricks-cli) token refresh that is still running at the deadline: if the access token expired during a long command and the refresh outlasts the rest of the upload budget (3s, or 1s for a hint-only upload), the process exits before the refreshed token is stored. If the server already rotated the refresh token, the next command fails to refresh untildatabricks auth login. Before,Uploadwaited and the refreshed token was saved.CLAUDE_CODE_SESSION_ID, or when the CLI cache is disabled or not writable). Theapps inittip and prompt print on every run where a detected agent has neither the shared skills nor the plugin for the working directory.version,completion,configureor mostauthsubcommands), the upload succeeds and telemetry is enabled. Because the install hint prints once per session per hour, a session whose hinted command resolves no auth logs no install hint event for that hour (documented inlibs/telemetry/protos/README.md).APPS_INIT_*events are also logged for humans who have an agent config dir; the event has no calling-agent field to tell them apart. An acceptedAPPS_INIT_PROMPTand a declined one look the same: the install the prompt starts logs no event..nextchanges/cli/telemetry-upload-timeout.md,.nextchanges/cli/apps-init-plugin-tip.md. The acceptance cases use bash andjqand were not run on Windows here; Windows and macOS were only cross-compiled and vetted.This pull request and its description were written by Isaac.