agoda-com / agoda-com/devfeedback-js

Ingestion API: data model changes required by the full dev-cycle events (#46)

Abierto
#47 3 comentarios 1 reacción 0 asignados Ver en GitHub
Lenguaje dominante
TypeScript
Estrellas
7
Forks
8
Métricas de merge de PR
Sin PR fusionados en 30 d

Descripción

Companion to #46, which extends the JS client from single compilations to the whole local dev cycle. Everything below is what the **ingestion API** has to change to accept and store what the client now sends. The client work is done; without these changes the new events are either rejected or land with their most useful columns dropped.

**Scope:** ingestion API + storage schema + dashboards. No client changes here.

---

## 1. A new event type and endpoint: `type: "command"`

The three existing endpoints (`/webpack`, `/vite`, `/rspack`) each accept one compilation-shaped payload. Lifecycle spans do not fit that shape, so they go to a new endpoint:

```
POST /command (client env var override: COMMAND_ENDPOINT)
```

Default URL is `http://compilation-metrics/command`, matching the existing convention.

The payload is `CommonMetadata` plus the fields below. Sample payloads for every phase are in [`examples/command.json`](https://github.com/agoda-com/devfeedback-js/blob/master/examples/command.json).

| Field | Type | Required | Notes |
| --- | --- | --- | --- |
| `type` | `"command"` | yes | discriminator |
| `phase` | enum | yes | `install` \| `codegen` \| `typecheck` \| `lint` \| `test` \| `build` \| `devserver` \| `clientready`. Only `install`, `devserver` and `clientready` are emitted today; the rest are reserved so adding them later is not another schema change. **Please store as a string, not a DB enum** — a new phase should not need a migration. |
| `command` | string | yes | e.g. `vite dev`, `rsbuild dev`, `npm install`, or the raw package manager user agent |
| `exitCode` | int | yes | `0` success, `130` SIGINT |
| `success` | bool | yes | |
| `signal` | string | no | `SIGINT` when the developer gave up waiting. **This is the field the "abandonment" analysis depends on.** |
| `errorCount` | int | no | reserved, not emitted yet |
| `timeTaken` | number | yes | inherited from `CommonMetadata`, milliseconds, the duration of the span |

### Install-specific

| Field | Type | Notes |
| --- | --- | --- |
| `packageManager` | `npm` \| `yarn` \| `pnpm` | all three are in use across our repos, so all three must be accepted |
| `packageManagerVersion` | string | |
| `coldInstall` | bool | `node_modules` absent beforehand |
| `lockfileChanged` | bool | lockfile hash before vs after |
| `measurementSource` | `preinstall` \| `postinstall` \| `npm-timing` | **how the number was obtained, and it matters for comparability.** `preinstall` is an exact span. `postinstall` is inferred from the package manager's process start time and ends when our package is linked, so it systematically undercounts. `npm-timing` is npm's own total. Do not aggregate across these without splitting by this field. |
| `npmTimers` | `map` | npm's per-phase and per-package timers, milliseconds. Unbounded key set (`build:run:install:node_modules/playwright`, `reify`, `idealTree`, …), client-capped at 500 entries. See §3. |

### Dev server / client specific

| Field | Type | Notes |
| --- | --- | --- |
| `prebundled` | bool | Vite only, **tri-state** — `true`, `false`, or absent when unknown. Absent must not be coerced to `false`; under Rolldown-based Vite the metadata file may not exist at all. |
| `domContentLoadedMs` | number | `clientready` phase |
| `firstContentfulPaintMs` | number | `clientready` phase |

---

## 2. `sessionId` on **every** event type

`CommonMetadata` gains one field, so this lands on `webpack`, `vite`, `vitehmr`, `rspack`, `rsbuild` **and** `command` payloads alike:

```
sessionId: string // uuid
```

This is the whole point of the exercise: it is what lets `install` → `devserver` → first `vitehmr` be stitched into one timeline and answer "how long from `git pull` to a working app?".

Ingestion changes needed:

- Add `sessionId` to every existing event table/schema.
- Backward compatible in both directions: old clients send no `sessionId` (accept and store null), new clients send one to old endpoints (must not be rejected as an unknown field). **Please confirm the current validation is additive-tolerant** — if unknown fields are rejected today, that is a blocker for rollout, not a nice-to-have.
- Index it. Every interesting query groups by it.

---

## 3. `npmTimers`: decision needed on storage shape

`npmTimers` is a wide, sparse, unbounded-key map — one key per npm phase plus one per package that runs an install script. Three options, roughly in order of my preference:

1. **JSON column on the install event.** Cheapest to land, query with JSON functions. Fine if these are analysed occasionally rather than charted continuously.
2. **Child table** `install_timer(event_id, timer_name, duration_ms)`. Best for "which package costs the most install time across the org, week over week", which is the question that justified collecting this at all.
3. Store only a known subset (`reify`, `idealTree`, `audit`, `build:*` aggregate).

I do **not** want option 3 — per the design conversation, prefer keeping the raw data and deciding what to aggregate later over precomputing and discovering we threw away the interesting bit. Option 2 if this is going on a dashboard, option 1 if not.

---

## 4. `spooledAt`: keep it as a first-class field

```
spooledAt?: number // epoch ms
```

Install events and SIGINT events are **spooled locally and delivered by the next dev server or build start**, because neither an install nor a dying process can afford to wait on the network. Delivery can therefore be minutes, hours, or (after a weekend offline) days after the event.

`spooledAt` is when the event was written to the local spool. Decision: **store it as a real column** rather than inferring lateness from ingest time minus `timestamp`.

- Inferring it conflates "delivered late" with "the build genuinely took that long" and with clock skew on the developer's machine.
- Dashboards should be able to filter to `spooledAt IS NULL` for real-time events without a heuristic.
- It costs one nullable bigint.

Note that for `measurementSource: "npm-timing"` events the client sets `spooledAt` to the mtime of npm's log file — i.e. when the install actually happened — since those are scraped after the fact by definition.

---

## 5. Rspack/Rsbuild endpoint correction

`rsbuild` and `rspack` events were being posted to the **webpack** endpoint, contradicting the README, which has always documented `/rspack`. The client now posts them to `/rspack`.

Ingestion side:

- Confirm `/rspack` accepts `type: "rsbuild"` as well as `type: "rspack"`.
- Anything downstream that reads Rspack data off the webpack endpoint needs repointing, and there will be a period where old client versions still send to webpack — **both paths need to work during the transition.**
- Worth a one-off check for how much existing "webpack" volume is actually rsbuild, since that skews any historical webpack numbers.

---

## Acceptance criteria

- [ ] `POST /command` accepts every phase in `examples/command.json` and rejects nothing valid.
- [ ] `sessionId` is stored on all six event types and is queryable/indexed.
- [ ] An old client (no `sessionId`, no `type: "command"`) still ingests successfully.
- [ ] A new client posting to an un-upgraded endpoint is not rejected for unknown fields.
- [ ] `measurementSource` is preserved and exposed, so install-duration aggregates can be split by it.
- [ ] `prebundled` distinguishes absent from `false`.
- [ ] `spooledAt` is stored as its own nullable column.
- [ ] `/rspack` accepts both `rspack` and `rsbuild`; the webpack path keeps working for older clients.
- [ ] `npmTimers` storage shape decided and implemented (see §3).

## Open questions for the ingestion side

1. Is current payload validation strict (rejects unknown fields) or additive-tolerant? This gates the rollout order — if strict, ingestion must ship before the client.
2. `npmTimers`: JSON column or child table (§3)?
3. Any retention or cardinality limit I should know about before we start writing one `command` event per dev server start per developer per day?

Guía de contribución

Abrir la guía de contribución

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.