Repository navigation
feat(app): split the service map by extra group-by expressions - #3632
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 2f65380 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Duplicate code analysisNo new duplicate code introduced by this PR. jscpd compared this branch against |
E2E Test Results✅ All tests passed • 466 passed • 1 skipped • 1270s
Tests ran across 4 shards in parallel. |
Nodes were one per ServiceName and edges one per client service -> server service, so a single slow or failing pod was averaged into its service. The service map now takes up to 3 extra SQL expressions through a groupBy URL param and a Group by editor; a node is the (ServiceName, dim0, dim1, dim2) tuple and an edge joins two such tuples. Both CTEs select each expression as toString(expr). The outer query selects every key column for both sides of the join as server_<col> / client_<col>, groups by the key tuples and excludes self-calls by comparing the full tuples, so two pods of one service calling each other are an edge while a node calling itself is not. With no expressions the query reduces to the previous one. Node labels gain a second line with the dimension values and the tooltip search links pin each value. The e2e seed gives every service two pods and the new spec covers grouping from the URL, from the editor, and the 3-expression cap.
2a154d1 to
7ab16e5
Compare
🟡 Tier 3 — StandardIntroduces new logic, modifies core functionality, or touches areas with non-trivial risk. Why this tier:
Review process: Full human review — logic, architecture, edge cases. Stats
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor None could be anchored to changed lines; they are listed below. Findings outside the changed lines3 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Deep Review✅ No critical issues found. This is a well-structured, heavily-tested change. The SQL-shape, aggregation, dimension-formatting, search-pinning, and size-gate behaviors each have focused unit tests, plus an e2e spec, and every item raised in prior review comments (NULL normalization via 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (9): correctness, testing, maintainability, project-standards, security, performance, kieran-typescript, adversarial, previous-comments. Testing gaps: None material — the new helpers, the size gate (including its reset-on-new-result behavior), the SQL shape for zero and multiple dimensions, and the end-to-end group-by flow are all covered. The one untested edge is the extreme-cardinality fetch/aggregate path noted in the P2 finding. No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
…join Each group-by expression is now selected as ifNull(toString(expr), '') in the span CTEs and compared the same way in the tooltip search links, so a NULL or missing attribute keeps unmatched callers in the node totals instead of dropping them from the WHERE, and the pinned search value matches what the map grouped on for array, numeric and operator expressions. The feature targets resource-level attributes: a span attribute is evaluated on the client span, i.e. with the callee's value, so it splits callers by what they call. The editor tooltip says so and aggregateServiceMapData has a test for that shape. The editor wrapper uses Mantine style props instead of a raw style object.
The time range is now set inside the form's submit callback, so an invalid group-by leaves the URL untouched instead of moving the time window; the e2e spec checks the groupBy param rather than the whole URL. The expression cap lives in one place: parseServiceMapGroupBy caps the URL param and seeds the editor, so a hand-edited URL with too many expressions shows exactly what the map is grouped by. The hook no longer slices, and the editor tooltip builds its limit from the constant.
| // the right set of spans. A node calling itself is not an edge, but two nodes | ||
| // of the same service (e.g. two pods) are, so the self-call exclusion compares | ||
| // the full key tuples. Ordering helps stable graph layout. | ||
| return chSql` |
There was a problem hiding this comment.
What happens when someone groups by a high-cardinality expression?
There's no LIMIT on the query, so SpanAttributes['http.url'] or TraceId
would return a node per value, so we could end up with thousands of nodes being added. Might be worth adding a max cap, possibly allowing the user to override this if they're that keen on blowing up their browser tab.
const MAX_SERVICE_MAP_NODES = 500;There was a problem hiding this comment.
Good call. Done in aa477ee with a lower default: maps above MAX_SERVICE_MAP_NODES = 100 are not laid out at all (the dagre layout and React Flow render are what lock the tab, not the query); an empty state names the node count and offers "Show anyway", which renders that result set once and gates the next one again. Applied before layout rather than as a SQL LIMIT because the GROUPING SETS rows mix node and edge rows, so a row limit would cut edges whose nodes never arrived. Checked locally with groupBy=SpanId: 1,676 nodes gated, rendered on click.
A high-cardinality group-by expression such as a URL or a span id yields one node per value, and the dagre layout plus React Flow render of thousands of nodes is what locks the tab. Maps above MAX_SERVICE_MAP_NODES are no longer laid out; an empty state names the node count and offers Show anyway, which renders that result set once and gates the next one again.
| @@ -693,3 +697,14 @@ describe('deriveDisplayMetrics', () => { | |||
| expect(m.latencyMs).toBeDefined(); | |||
| }); | |||
There was a problem hiding this comment.
🔵 minor — The 'Show anyway' test checks a boolean helper, not the gate that resets on new results
Add a component test for ServiceMapPresentation in ServiceMap.tsx. Render it with more than 100 nodes, click service-map-show-anyway, then rerender with a new services Map and assert service-map-too-many-nodes comes back. As written, the test would still pass if showAnywayFor === services were replaced by a plain boolean that never resets.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Done in 72963d2: the gate now lives in useServiceMapSizeGate, and its test renders the hook with an oversized map, calls Show anyway, re-renders with the same Map (still shown) and then with a new Map of the same size (gated again). A plain boolean that never resets fails that last step. Rendering ServiceMapPresentation itself would need React Flow and dagre under jsdom, which the hook test avoids.
The gate moves into useServiceMapSizeGate so its behaviour is testable with renderHook: an oversized map is held back, Show anyway releases that result set, and a new result set of the same size is held back again.
| </div> | ||
| <Text size="xxs">{serviceName}</Text> | ||
| {dimensionsLabel ? ( | ||
| <Text |
There was a problem hiding this comment.
🔵 minor — Long dimension labels on nodes are never truncated, so they spill over neighbouring nodes
Clamp the node's dimension line the way the tooltip already does (e.g. lineClamp={1}, a maw near the layout width, and title={dimensionsLabel} for the full text). getGraphLayout in ServiceMap.tsx gives dagre a fixed 80px node width, and .serviceNode in ServiceMap.module.scss sets no max-width, overflow or white-space rule. A typical label from three group-by values (checkout-7d9f8b6c5-x2k4p · 1.4.2 · us-east-1) is several hundred px wide, so it runs into nodes in the next rank. The tooltip line at ServiceMapTooltip.tsx is already clamped.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
The service map draws one node per
ServiceName, so a service running on several pods is one circle and a single slow or failing pod is averaged into its siblings: one error-heavy pod among three shows on the service node at a third of its error rate. The map can say which service is unhealthy, never which instance.This adds a Group by box next to the span filter. Up to 3 extra SQL expressions, e.g.
ResourceAttributes['k8s.pod.name'], split every node into (service, values…) tuples; edges join those tuples, so two pods of one service calling each other now draw an edge while a node calling itself still does not. The expressions live in agroupByURL param, each node shows the values as a second label line, and the tooltip search links pin them. With the box empty, the SQL and the map are unchanged.In
useServiceMapboth CTEs select each expression asifNull(toString(expr), ''); the outer query selects every key column for both sides of the join asserver_<col>/client_<col>and groups by the key tuples. The feature is meant for resource-level attributes (pod, node, version, environment): a span attribute is evaluated on the client span too, i.e. with the callee's value, so it splits callers by what they call; the editor's tooltip says so andaggregateServiceMapDatahas a test for that shape. Maps with more than 100 nodes are not laid out; an empty state names the count and offers Show anyway, so a high-cardinality expression cannot lock the tab by accident. "Focus on this service" still scopes byServiceName, so focusing a pod node shows every pod of that service; the single-trace side panel is unchanged.Most of the code was written with Claude Code; I reviewed it and tested it against a local stack with multi-pod traces.
Screenshots or video
Clicking on a node, it prefills the query with the right selectors:
How to reproduce
Starting point:
yarn app:dev:local, onboarding → Connect to Demo Server, then open Service map. The demo runs one instance per service, so a resource attribute shows up as a second label line rather than as a split.On
main:On this branch:
ResourceAttributes['service.version'], press Escape to close the autocomplete, then Enter. The URL gainsgroupBy=…and each node shows its version under the service name.a, b, c, dand press Enter: the box shows "Up to 3 group by expressions" and the URL does not change.How to test on Vercel preview
Preview routes: /service-map
Steps:
service-map-group-by-input), typeResourceAttributes['service.version'], press Escape, then press Enter.groupBy=and that nodes show a second, muted label line (data-testidservice-map-node-dimensions).a, b, c, dand press Enter.groupByvalue.References