feat: add auth telemetry lifecycle traces#994
Conversation
EhabY
force-pushed
the
feat/auth-telemetry-lifecycle
branch
from
5df69b2 to
23f7ba4
Compare
- Revert cliCredentialManager from `#` fields back to `private` to cut diff
- Restore parallel file/keyring deletion in deleteToken
- Drop the failure-category return threaded up to logout; record credential
failures on the auth.credential.clear span at the source instead
- Pass span last and grouped ({ signal, span }) into deleteKeyringToken
- Remove the loginMethods identity map, dead AuthLoginMethod/AuthLoginReason,
unused "direct" source, and collapse the record-* helpers to one
- Drop redundant setMethod calls now that traceLogin records method on return
- Rename commands.test.ts -> commands.telemetry.test.ts
- Fix uriHandler tests broken by the telemetry service dependency
DanielleMaywood
left a comment
DanielleMaywood
left a comment
There was a problem hiding this comment.
Just a stylistic question about the async function drop. Not blocking, just curious.
commands.telemetry.test.ts has many as unknown as. I understand why we're leaning to them but it does pain me and it'd be nice if we could have a good solution that doesn't require that.
| public async storeToken( | ||
| public storeToken( |
There was a problem hiding this comment.
How come we've dropped the async? I might just have a knowledge gap with JS practices but I'd have assumed we'd keep this function as async and call await on our credentialTelemetry.traceStore.
There was a problem hiding this comment.
We have lint rules that remove the async when there are no await, it might still be an async function that returns a promise but in this case we just have to return said promise without waiting
There is no advantage to just calling doing return promise over return await promise unless we want to catch errors, then the latter makes sense. Otherwise it's just noise
Make telemetry dimensions explicit at every call site: suspendSession and clearDeployment now require a reason instead of defaulting (the suspendSession default was never used in production), and CliCredentialManager requires its TelemetryReporter rather than silently falling back to the noop reporter. Replace the scenario DSL in commands.telemetry.test.ts with plain it() blocks on a shared setup() that reuses createTelemetryHarness, and drop the unused clearDeploymentError option.
EhabY
force-pushed
the
feat/auth-telemetry-lifecycle
branch
from
4c71534 to
9f32eec
Compare
2 participants
Summary
Closes #984
Validation
pnpm format:checkpnpm lintpnpm typecheckpnpm test:extension ./test/unit/core/cliCredentialManager.test.ts ./test/unit/login/loginCoordinator.test.ts ./test/unit/core/cliManager.test.ts ./test/unit/deployment/deploymentManager.test.ts ./test/unit/instrumentation/auth.test.ts ./test/unit/commands.test.tsImplementation notes
auth.loginspan so command/auto-login/switch-deployment sources and URL-prompt cancellation are captured once.LoginCoordinator.ensureLoggedIn({ traceLogin: false })is only used from that command-owned span to avoid emitting a second direct-login span for the same attempt.Generated by Coder Agents for @EhabY.