Add pipeline processing and completion to time series stream, add pipeline latency graph to Performance tab - #6650
Conversation
|
Is there a reason not to always show p99? The checkbox is awkward. It's worth having a link to https://docs.feldera.com/pipelines/latency/ Also, 3ms + 3ms doesn't make sense. Completion latency includes step latency. |
In the title I subtract the current completion from step latency to show the "internal Feldera" latency and sink latency separately. I can revert to just showing the absolute latency values. Graphs and the tooltips show the absolute values, of course
I only made it optional to avoid overloading the chart by showing 4 lines in the graph by default. I am happy to show p99 unconditionally if that's the consensus |
|
To make the title less confusing I could reword it to "X ms processing + Y ms output" |
Easier so it corresponds to the graph: |
|
Ok, putting completion first, LMK if I should switch them around (again, to highlight Feldera itself is fast) |
|
The other way around is fine too. |
mythical-fred
left a comment
There was a problem hiding this comment.
Nice addition. The backend pieces (histogram quantile(), per-connector aggregation, four new time-series fields with skip_serializing_if) and the frontend (adaptive formatDuration, p99 toggle, tightened baseline y-axis when hidden, oscillation-safe compact title) all read cleanly, and both sides have thorough tests. CI still pending at review time, but the code is signed-off and free of AI-tool trailers. Approving with one design question and two nits.
Design question — "p99" is p99 of medians, not tail latency
ControllerStatus::latency_percentiles collects each connector's median into a vector and takes p50/p99 across that vector (stats.rs lines ~757-793). With the number of input connectors a typical pipeline has (often 1-3), the "p99 across connectors" line is arithmetically identical to the max of medians and does not reflect within-connector tail latency at all. The graph legend labels it p99 and the tooltip renders it prominently, which is likely to be read as "the slowest 1% of records", not "the slowest median among connectors".
Two alternatives worth considering:
- Per-connector
p99first, then aggregate. Pullquantile(0.99)from each connector's histogram (you already added the method) and take the max (or p99) across connectors. That way each series answers a well-defined tail-latency question even with one connector. - Combined-histogram approach. Fold the per-connector histograms into one before quantile-ing. Buckets are exponential and shared, so you can add them element-wise.
Not a merge blocker — the graph still surfaces slow connectors, which is useful — but the current label sets user expectations the metric can't meet. At minimum, the tile title / axis label could be honest about it ("slowest connector median" or similar).
Nits (non-blocking)
ExponentialHistogramSnapshot::quantilereturns the lower bound of the containing bucket (bucket_to_range(index).start()). For high-magnitude latencies the exponential bucket widths get proportionally large, so a value in bucket[900_000, 999_999]reports as900_000— a ~10% systematic underestimate on the tail exactly where accuracy matters most. Cheap fix: return the midpoint or the upper bound; the existing testquantile_known_distributionwould need one line updated. Or at least call out the bias in the rustdoc so a future reader doesn't chase a phantom "why is p99 lower than reality" bug.niceLatencyMax([])returns1000(µs), so an empty latency series draws a 0-1ms axis. Fine for the graph's cold-start moment, but combined with thehasDataboolean you compute and never surface to the UI, a "no latency samples yet" state would be more honest than a fake 1ms axis. Consider rendering a placeholder in the tile when!hasData.
CI is still pending; if any check fails, please re-ping.
|
|
||
| /// Latency percentiles (microseconds) across input connectors, for the | ||
| /// time-series graph. Each connector contributes its own median latency, | ||
| /// we then take p50 and p99 across connectors, for both |
There was a problem hiding this comment.
Design nit: this is "p99 across connectors of their medians", not "p99 of record latencies". With N input connectors typically small (1-3), the p99 line is arithmetically the max of medians and does not reflect the tail distribution any connector actually sees. Consider (a) calling quantile(0.99) on each connector's histogram and aggregating that, or (b) folding the per-connector histograms into one before quantile-ing. See the review body.
| let mut cumulative = 0u64; | ||
| for (index, count) in self.buckets.iter().enumerate() { | ||
| cumulative += *count; | ||
| if cumulative >= rank { |
There was a problem hiding this comment.
Reporting the bucket's lower bound biases the estimate downward, and the bias scales with the value: a 950ms sample lands in [900_000, 999_999] and reports as 900ms — a ~10% underestimate on the p99 tail where accuracy matters most. The bucket midpoint (or upper bound) is a cheaper reporting choice with lower expected error; alternatively, at least document the systematic bias in the rustdoc so a reader isn't confused why p99 always looks fast.
|
|
@Karakatiza666 let me know when this is ready again for review after you addressed the comments. |
|
An aggregate completion latency graph that somehow summarizes across all connectors may be misleading. Different data sources may have very different latency behaviors, e.g., a Kafka connector that ingests many small messages can have latency in milliseconds, while a Delta connector that processes an occasional large merge can take multiple seconds or minutes. I don't know what p99 even means here, since we have two dimensions: N connectors x M seconds reporting window. |
mythical-fred
left a comment
There was a problem hiding this comment.
APPROVE-carry.
Single new commit 825e805e on the latency-graph slice: legend condensed to the two p50 series (family = hue, percentile = line style — colour-blind & grayscale friendly), a new circle-help SVG glyph feeds a Popover tooltip that explains what each series is, and a formatDuration utility with tests for the µs/ms/s ladder and non-finite fallback. The rest is the icon-font rebuild that comes with adding one glyph.
Nothing to block on. Rust adapter + storage code is untouched from prior tip. Prior APPROVE at 9688f44 stands.
|
A new graph was scrapped in favor of per-connector latency value, as discussed with @ryzhyk : The color of the value text is interpolated from black to red within minimum and maximum latency across all input connectors, but the maximum taken is no less than 5x of the minimum latency. |
825e805 to
26ce1a5
Compare
|
Per connector, changed from the median to p99 |
[web-console] Add pipeline latency graph to Performance tab Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
…ng the chart Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
…y value Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
…nds coming from the API Signed-off-by: Karakatiza666 <bulakh.96@gmail.com>
11b0f76 to
502f816
Compare
| /// [`ExternalInputEndpointMetrics::processing_latency_p99_micros`] for what | ||
| /// the window covers. | ||
| fn processing_latency_p99_micros(&self) -> Option<u64> { | ||
| self.processing_latency_micros_histogram |
There was a problem hiding this comment.
what data is in this histogram?
There was a problem hiding this comment.
This processing_latency_micros_histogram holds, per input endpoint, a rolling set of per-batch durations in microseconds: how long each ingested batch took to get through the circuit - includes ingest, excludes output connector latency (completion). It pre-dates this PR, I only calculate 99th percentile from it.
| /// | ||
| /// Does not account for completion latency. | ||
| /// | ||
| /// Taken over the endpoint's sliding histogram, which holds the 10,000 most |
There was a problem hiding this comment.
If this is a value over 10 minutes it should not be displayed as a graph which is a function of time
There was a problem hiding this comment.
The PR no longer introduces a graph. Instead, it shows per-connector p99 latency as a new column in the input connector metrics table
There was a problem hiding this comment.
I hope there is some help somewhere allowing people to know which interval this is computed over
There was a problem hiding this comment.
There is, there is a screenshotin the PR description, here is the tooltip:
| @@ -0,0 +1,16 @@ | |||
| import type { SameNullability } from '$lib/types/common/nullable' | |||
|
|
|||
| declare const microsecondsBrand: unique symbol | |||
There was a problem hiding this comment.
This is a way to define a field type that cannot be reproduced outside the module, enabling opaque types that can not be instantiated except by using some type of constructor from the same module
| class="fd fd-circle-help text-[16px] leading-none text-surface-600-400" | ||
| ></span> | ||
| </span> | ||
| <Tooltip placement="top" class="max-w-sm text-wrap" |
There was a problem hiding this comment.
@ryzhyk left this comment
An aggregate completion latency graph that somehow summarizes across all connectors may be misleading. Different data sources may have very different latency behaviors, e.g., a Kafka connector that ingests many small messages can have latency in milliseconds, while a Delta connector that processes an occasional large merge can take multiple seconds or minutes.
What is the verdict on this objection?
There was a problem hiding this comment.
It has been resolved: instead of displaying a single graph I now display per-connector p99 latency. This avoids aggregating across connectors which was the main objection
| import type { OpenApiSchemaObject, UserConfig } from '@hey-api/openapi-ts' | ||
|
|
||
| /** Hand-written type substituted for a marked schema. */ | ||
| export type CustomType = { |
There was a problem hiding this comment.
what custom types are you using?
In general I don't like this kind of programs which rewrite other stuff and load dynamically, they are not discoverable using normal tools and can cause tough to troubleshoot problems
why is this needed really? why wasn't it needed until now?
There was a problem hiding this comment.
These are discoverable by a text search for the Microseconds type. This plugin allows using a custom type alias - Microseconds, which is just a number at runtime - to mark fields from the API in the auto-generated OpenAPI bindings. Using custom type wrappers is something you already mentioned a while ago; this way I can use these right on the API boundary, instead of having to wrap generated functions and types to override the filed type.
Added computing per-connector p99 processing (not completion) latency from an existing histogram.
Added the latency column for input connectors metrics table where values get colored red based on how much higher the latency is. The minimum magnitude the high latency needs to reach to be displayed in a saturated red color is 500% of the connector with the lowest latency - so that a small spread in latencies across connectors does not cause connectors with slightly higher latency in absolute terms to be colored bright red. The 500% minimum is configurable.
====== BELOW IS THE ORIGINAL PR DESCRIPTION, NO LONGER RELEVANT =======
Every pipeline already stores distribution of per-record latencies for every connector, in buckets. This PR calculates a median latency per connector based on these buckets, and then aggregates latency across all connectors as p50 and p99, for input-to-processing and input-to-completion latency. The whole-pipeline step latency metric is not used for calculation.
I chose to show two latencies because I want users to distinguish between Feldera's internal latency and completion latency, which is impacted by the output sink performance
Testing: manual, added unit tests