React UI: Add Exemplar Support to Graph#8832
Conversation
|
Awesome, thank you! Gonna take a closer look tomorrow :) |
juliusv
left a comment
There was a problem hiding this comment.
Pretty awesome! A couple of comments, feel free to push back if some stuff I say doesn't make sense.
Separately, it would be great if someone else who has actually worked with exemplars before (@gouthamve?) could give an opinion on not the code, but the actual feature / usage.
| throw new Error(query.error || 'invalid response JSON'); | ||
| } | ||
|
|
||
| if (showExemplars) { |
There was a problem hiding this comment.
I get the intent to group everything graph-related under the switch case above, but at the point where we are actually deciding whether to run a query at all or not, I wonder if it wouldn't be more straightforward to understand if we just omitted the extra variable and just did this here:
| if (showExemplars) { | |
| if (this.props.options.type === 'graph' && this.props.showExemplars) { |
| resultSeries = result.length; | ||
| } | ||
| let query; | ||
| let exemplars; |
There was a problem hiding this comment.
As a side note, we should really start typing these responses, but that's not your job here :)
There was a problem hiding this comment.
I'd be down to work on that, just in different PR.
| } | ||
|
|
||
| if (showExemplars) { | ||
| exemplars = await fetch(`${this.props.pathPrefix}/${API_PATH}/query_exemplars?${params}`, { |
There was a problem hiding this comment.
Hmm, we're sending an extra step parameter here, which currently doesn't cause a problem, but is a bit unclean, as it depends on the current accidental fact that the API doesn't return 400 Bad Request errors when it encounters extra parameters. To still keep things simple and not require a new URLSearchParams object, how about just doing params.delete('step'); before this line? Also kinda makes that explicit to the reader.
| } | ||
|
|
||
| if (exemplars && exemplars.data) { | ||
| query.data.exemplars = exemplars.data; |
There was a problem hiding this comment.
I'd prefer using a completely separate exemplars: Exemplar[] state field for the exemplars, because right now query is just a dumb data transfer object that contains exactly the query response as delivered by the API. Adding extra stuff to it that wasn't returned in the same query could be confusing to readers.
| ...chartData.exemplars.filter(exemplar => { | ||
| series: for (const i in selected) { | ||
| for (const name in chartData.series[selected[i]].labels) { | ||
| if (exemplar.labels[`Series: ${name}`] !== chartData.series[selected[i]].labels[name]) { |
There was a problem hiding this comment.
It's a bit too surprising IMO that this depends on the exact magic string formatting (that one would assume would only be consumed by humans) from another place. I wonder if the series labels wouldn't better be stored in a separate set of labels (seriesLabels?) in the exemplar? But just thinking out loud here...
There was a problem hiding this comment.
Yup, that actually works great! It was my false belief that flot wouldn't allow any fields in the datapoint outside of its defined structure, but this isn't Go...
|
I can comment on, or get comments from others @ Grafana Labs, regarding usage sometime this week. |
|
One prominent use I see is tracing and the display of |
| .map(({ seriesLabels, exemplars }) => { | ||
| const newLabels: { [key: string]: string } = {}; | ||
| for (const label in seriesLabels) { | ||
| newLabels[`Series: ${label}`] = seriesLabels[label]; |
There was a problem hiding this comment.
Do we still need this merging of exemplar labels with series labels here now? I think it'd be cool to not prefix each of them with Series: but modify the tooltip code in GraphHelpers.tsx to have a separate labels section that is only titled "Series labels:" once, and then lists the series labels separately.
Yeah, that would totally make sense. We could install an onclick handler (like in https://www.flotcharts.org/flot/examples/interacting/index.html) and show more permanent info above oder underneath the plot area about the clicked exemplar, including possibly a link to a tracing system (I would imagine in the future that this could be provided by a new command-line flag). |
|
My question would be then what would we copy, the names of the labels and their values, or just the values? Only the first, or all the labels? I think it's logical to copy a comma+space separated list of label values and let the user delete what they don't want, but I'm open to other ideas. |
In which context do you mean this? In showing extra info on a trace outside of the graph area? |
|
Ah, I see I misunderstood your previous comment. I thought the onclick handler would be used to copy the content, not to show more information somewhere else. It makes more sense now, thanks :) |
|
Ah ok, yes :) |
| <span style={{ fontSize: '17px' }}>Selected exemplar:</span> | ||
| <div className="labels mt-1"> | ||
| {Object.keys(selectedLabels).map((k, i) => ( | ||
| <div key={i} style={{ fontSize: '15px' }}> |
There was a problem hiding this comment.
For some reason, Tailwind wasn't applying these font-sizes (as well as some other properties) so I had to resort to style props.
There was a problem hiding this comment.
@LeviHarrison We're not using Tailwind, so that might be the reason? We do have a number of CSS / SCSS (for the theme stuff) files though which we use in addition to inline styles. So you could use e.g. https://github.com/prometheus/prometheus/blob/main/web/ui/react-app/src/themes/_shared.scss
There was a problem hiding this comment.
Style-wise I'd prefer the font sizes and styles to be similar to the legend items, and instead of floating the exemplar details to the right, how about inserting another full-width section above the legend (equally indented, etc.)? Keeps things simpler / more predictable in the face of shifting window + legend widths.
|
Here's what the popup looks like
|
|
Hmm, I can reliably produce the |
| return isNaN(val) ? null : val; | ||
| }; | ||
|
|
||
| const exemplarSymbol = (ctx: CanvasRenderingContext2D, x: number, y: number) => { |
There was a problem hiding this comment.
Btw. I just played around and find the default points (when omitting the custom symbol function) more beautiful, plus they have a nice hover effect :) Did you mainly switch to a custom method for size reasons (which should be controllable by the radius property)?
There was a problem hiding this comment.
Yes, it was mainly for size reasons. I don't believe you can control the radius per-point, only for all of them (series and exemplars), so the custom symbol was the only option. The square also creates distinction between it and a regular series point.
There was a problem hiding this comment.
Hmm... so I can definitely increase the radius of only the exemplar circles, and normal series are never drawn as circles, only as line segments, so there should at least be a distinction already.
points: { show: true, radius: 4 },Gives me:
test-2021-05-23_09.28.53.mp4
The nice thing about this option is that it works with the existing hover plugin and thus meets the user expectation of indicating which exemplar (or other point) you are currently hovering over.
On the custom drawing alternative front I also experimented with making the exemplars a bit more pretty (IMO), like this:
const exemplarSymbol = (ctx: CanvasRenderingContext2D, x: number, y: number) => {
// Center the symbol on the point.
y = y - 3.5;
// Correct if the symbol is overflowing off the grid.
if (x > ctx.canvas.clientWidth - 59) {
x = ctx.canvas.clientWidth - 59;
}
if (y > ctx.canvas.clientHeight - 37) {
y = ctx.canvas.clientHeight - 37;
}
ctx.translate(x, y);
ctx.rotate(Math.PI / 4);
ctx.translate(-x, -y);
ctx.fillStyle = '#92bce1';
ctx.fillRect(x, y, 7, 7);
ctx.strokeStyle = '#0275d8';
ctx.lineWidth = 1;
ctx.strokeRect(x, y, 7, 7);
}That gives us:
test-2021-05-23_09.33.47.mp4
But the downside is that hovering doesn't show a highlight. I guess we could build our own specific hover functionality, like the hover plugin (http://www.flotcharts.org/flot/source/jquery.flot.hover.js) does it for circles.
I think either of those solutions is fine as a start and we shouldn't block the PR merge on more though.
There was a problem hiding this comment.
Weird, I think I tried changing the radius and it messed up the lines, but cool that it works! I think that your custom symbols fit in a lot better and look really good, so I'll go ahead and add that in. Hover can definitely come later.
So the good news is, that error seems to even be unrelated to this PR, as I'm getting it on |
|
Yeah, I've got that one too. The good news is that I haven't seen any actual effect on the graph because of them, and when in a production setup no one will see them anyway (unless they look in the console). I've done some debugging and it seems that an It seems that we have flot |
|
@LeviHarrison Yeah, let's ignore the existing JS error for now then :) |
|
@cstyan Any news on the usage front? Would be great to get quick feedback from someone actually using exemplars! |
09de725 to
d2fa7bb
Compare
| if (y > ctx.canvas.clientHeight - 40) { | ||
| y = ctx.canvas.clientHeight - 40; |
There was a problem hiding this comment.
I had to change these values to keep the symbols from overflowing off the bottom, but other than that it's a direct copy of your comment. Looks really good!
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: GitHub <noreply@github.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev> Co-authored-by: Julius Volz <julius.volz@gmail.com> Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
|
@juliusv It was a quick one so I just got it out of the way. Also, maybe we should rename the |
|
@juliusv Just a friendly nudge on this if you have time. |
|
Yep, taking another look now! |
|
Awesome, seems to work great now. The one test seems to be still failing in an unrelated timeout, but I can't re-run it because apparently I don't have the permissions in CircleCI because it's in your private fork. Are you able to rerun it? |
|
Whoops, I had turned CircleCI on for my fork while working on a CI change. Should run on our installation now. |
|
There we go! |
|
👍 Woot, thanks! |
|
I'll try to do some styling follow-ups before the release. |
|
List of (maybe) follow-up things:
Other things that spawned from this
|
This PR adds exemplar support to the React UI graph. Exemplars can be:
(Partially) implements #8797
One thing that I still can't figure out is this error:
Uncaught TypeError: Cannot read property 'save' of null at drawOverlay (:3000/static/js/main.chunk.js:12297)when resizing the graph substantially and other big shifts. It has to do with the exemplar symbol, and as far as I can see it doesn't seem to have any effect. I suppose we could just leave it in, but it may be nice to check for undefined in that section of flot to avoid the message.Demo
I've found that the fastest way to get exemplars is through Grafana's Observability Demo App Docker Compose Demo. After cloning the repo and following the docker compose setup instructions, go to
production/docker-composeand rundocker-compose up tempo db app loadgen prometheus. Then start the react dev server in Prometheus. The two queries used in the video arehistogram_quantile(.99, sum(rate(tns_request_duration_seconds_bucket{}[1m])) by (le))(from this article) andtns_request_duration_seconds_bucket.exemplar-demo.mp4