feat: add instrumentaion to first mile journey#9193
Conversation
8afef7c to
61c55d0
Compare
792020e to
1c4a5cf
Compare
ericpgreen2
left a comment
There was a problem hiding this comment.
1. Consider centralizing telemetry in pushState/popState rather than sprinkling it through transition()
The state manager already knows everything needed to derive the telemetry payload: the current state (schema, connector, step) and the direction (forward vs. back). Instead of instrumenting each case branch individually, the telemetry could fire automatically from pushState and popState, with a mapping from state → action + fields. This would eliminate the scattered inline calls, make it impossible to miss a step, and keep the telemetry concern in two focused methods.
There's an existing TODO at BehaviourEventHandler.ts:19 acknowledging the broader telemetry layer verbosity. This PR is a good example of why that simplification is worth doing — the handler → factory → service chain requires a new method at each layer for what is conceptually "fire event with action name and some fields."
2. Telemetry gap: no event fires for the SelectConnector step
The old code in AddAssetButton.svelte fired a SourceAdd event when the user opened the add-data modal. That call is now removed. The new instrumentation fires step events inside the state machine, but the post-transition switch (AddDataStateManager.svelte.ts:207-249) only covers CreateConnector, CreateModel, ExploreConnector, and Import. The SelectConnector step — the first screen most users see — fires no event.
This means there's no way to tell how many users enter the flow and immediately close it, which seems like an important drop-off point given the stated goal of "know when users fall off."
3. fireErrorEvent — the step parameter vs. this.state.step for field population
fireErrorEvent accepts a step parameter (used in the event payload) but always reads this.state.step to decide which fields (schema, connector) to populate. In GenerateDashboardStatus.svelte:100, it's called with importStep (an ImportDataStep like "create-model"), while this.state.step is AddDataStep.Import. This is correct — the sub-step goes in the event, and the broader state gives context — but the dual meaning of "step" is confusing. Consider renaming the parameter to subStep or adding a brief comment explaining the distinction.
4. Minor: AddDataConfig mixes UI and telemetry concerns
AddDataConfig now holds both UI flags (welcomeScreen, importOnly) and telemetry fields (medium, space, screen). This works since all telemetry fields are optional, but if telemetry config grows, the type becomes muddled. A nested telemetry?: { medium, space, screen } would keep concerns separate.
Developed in collaboration with Claude Code
We need to know when users fall off. So adding instrumentation to identify step transitions, including errors and going back in the flow.
Checklist: