agoda-com / agoda-com/devfeedback-js

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

Đang mở
#47 3 bình luận 1 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
TypeScript
Star
7
Fork
8
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

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?

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.