Show a trace viewer in the execution page with timeseries data #7889

bduffany · 2024-11-13T04:49:53Z

Show a trace viewer in the execution page including event spans (flame chart) and timeseries data
- The trace is built on the server from the ExecutedActionMetadata and is streamed to the client over the /file/download endpoint using the JSON Trace Event Format
There are a couple reasons to use the trace viewer:
- It's more consistent with the invocation Timing profile and lets us get rid of some custom UI code for the execution timeline
- It already supports rendering nice timeseries charts
- It's interactive
- It will allow adding support for more fine-grained trace spans (e.g. download snapshot). Jaeger trace spans would also be super useful to display in this chart (if the execution was traced)
Only CPU timeseries data is shown for now, but memory usage, and IO rops, wops, wbytes, rbytes are in the works.

bduffany · 2024-11-13T16:33:18Z

server/util/trace_events/trace_events.go

+//
+// The format is documented here:
+// https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAySU
+package trace_events


I originally tried using https://github.com/omaskery/teffy instead of writing this package, but ultimately didn't like how it turned out:

The library nicely models the Trace Event Format from a correctness standpoint. It "makes invalid states unrepresentable" to a strict degree, splitting out the different "types" of events into separate structs rather than using a single flat struct with a "type" field (i.e. the "ph" / "phase" field in the JSON object). But the library heavily relies on struct embedding, which I found overly verbose and confusing since it suggests nesting that doesn't actually exist in the format. (It's confusing both when reading the call-sites and when reading the library implementation.) The JSON representation is also hidden in a separate "io" package which made it harder to mentally map the structs to the actual Trace Event Format. For example, the Counter event has a Values field which actually maps to "args" in the JSON representation, which was confusing to me and took a minute to grok.

The StreamingWriter implementation doesn't append newlines between each JSON object which makes it non-browser-friendly. In the browser we rely on the newline to separate objects, since the browser doesn't have a streaming JSON parser built in. (This is somewhat minor though and could be patched in if we really wanted it.)

jdhollen

I feel like @tylerwilliams has more opinions about the structure of file requests in buildbuddy_server than i do, so i'm really just focusing on the ts stuff, which all seems fine, just some dumb little nits.

jdhollen · 2024-11-13T17:30:58Z

app/invocation/invocation_action_card.tsx

+        "stream"
+      )
+      .then((response) => {
+        if (!response.body) throw new Error("failed to read profile: response body is null");


personal pet peeve: could be empty, probably is undefined.

tsc says the type of response.body is ReadableStream<Uint8Array<ArrayBufferLike>> | null in this case, so I think this is correct. Checked === null explicitly though to limit the type-narrowing magic here

jdhollen · 2024-11-13T17:34:27Z

app/invocation/invocation_action_card.tsx

+  }
+
+  getExecutionId() {
+    return this.props.search.get("executionId") || this.state.executionId;


this condition feels backward to me? i guess they shouldn't mismatch and one just shows up later?

Added a clarifying comment

jdhollen · 2024-11-13T17:36:36Z

app/invocation/invocation_action_card.tsx

-          })}
+          {this.state.profile ? (
+            <TraceViewer profile={this.state.profile} fitToContent filterHidden />
+          ) : this.state.profileLoading ? (


order seems wrong to me here (i.e., we should always show a spinner if profileLoading == true)--i assume it's because we always clear profile when we have something pending.

yeah, the logic is technically correct but I do think it's more clear to swap the order here (done)

jdhollen · 2024-11-13T17:50:35Z

app/trace/trace_events.ts

@@ -62,11 +63,14 @@ const TIME_SERIES_EVENT_NAMES_AND_ARG_KEYS: Map<string, string> = new Map([
  ["System load average", "load"],
  ["Network Up usage (total)", "system network up (Mbps)"],
  ["Network Down usage (total)", "system network down (Mbps)"],
+
+  // Executor
+  ["CPU usage", "cpu"],


maybe update the comment above to explain where these values come from + why we don't need "CPU Usage" for "Bazel" (which has "CPU usage (Bazel)") but do for "Executor"

I added some more detail to the comments but not sure if I totally understood the concern - lmk if it's still unclear?

btw I don't know the original context for this (maybe @luluz66 remembers) but I think we could potentially remove this explicit list and just render all of the timeseries, which are identified by "ph": "C" ("Counter") in the JSON objects. (Spans have "ph":"X" and timeseries have "ph":"C")

Yeah. this list is used as a filter when parsing trace events.

jdhollen · 2024-11-13T17:52:02Z

app/trace/trace_viewer_model.ts

+  const panels = [
+    buildEventsPanel(trace.traceEvents, fitToContent),
+    buildLinePlotsPanel(trace.traceEvents, fitToContent),
+  ].filter((panel) => panel.sections.length);


what does it mean to have a panel with no sections / why do we need to guard for it?

Added a comment

bduffany force-pushed the execution-ui-updates branch 2 times, most recently from 5cba57a to a7368f6 Compare November 13, 2024 05:01

bduffany requested review from jdhollen, tylerwilliams and siggisim November 13, 2024 15:21

bduffany marked this pull request as ready for review November 13, 2024 15:24

bduffany marked this pull request as draft November 13, 2024 15:24

bduffany force-pushed the execution-ui-updates branch from a7368f6 to 31e2dd1 Compare November 13, 2024 16:25

Show a trace viewer in the execution page

c24e1ed

bduffany force-pushed the execution-ui-updates branch from 31e2dd1 to c24e1ed Compare November 13, 2024 16:27

bduffany commented Nov 13, 2024

View reviewed changes

bduffany marked this pull request as ready for review November 13, 2024 16:43

bduffany changed the title ~~Show a trace viewer in the execution page~~ Show a trace viewer in the execution page with timeseries data Nov 13, 2024

jdhollen reviewed Nov 13, 2024

View reviewed changes

bduffany requested a review from jdhollen November 13, 2024 18:23

bduffany force-pushed the execution-ui-updates branch 3 times, most recently from 1528328 to dffb288 Compare November 13, 2024 18:34

Feedback

adaa8d3

bduffany force-pushed the execution-ui-updates branch from dffb288 to adaa8d3 Compare November 14, 2024 00:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show a trace viewer in the execution page with timeseries data #7889

Show a trace viewer in the execution page with timeseries data #7889

bduffany commented Nov 13, 2024 •

edited

Loading

bduffany Nov 13, 2024 •

edited

Loading

jdhollen left a comment

jdhollen Nov 13, 2024

bduffany Nov 13, 2024 •

edited

Loading

jdhollen Nov 13, 2024

bduffany Nov 13, 2024

jdhollen Nov 13, 2024

bduffany Nov 13, 2024

jdhollen Nov 13, 2024

bduffany Nov 13, 2024

bduffany Nov 13, 2024 •

edited

Loading

luluz66 Nov 13, 2024

jdhollen Nov 13, 2024

bduffany Nov 13, 2024

Show a trace viewer in the execution page with timeseries data #7889

Are you sure you want to change the base?

Show a trace viewer in the execution page with timeseries data #7889

Conversation

bduffany commented Nov 13, 2024 • edited Loading

bduffany Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

jdhollen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bduffany Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bduffany Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bduffany commented Nov 13, 2024 •

edited

Loading

bduffany Nov 13, 2024 •

edited

Loading

bduffany Nov 13, 2024 •

edited

Loading

bduffany Nov 13, 2024 •

edited

Loading