[APM] Chart units don't update when toggling the chart legends

[cauemarcondes]

closes #74830

[elasticmachine]

Pinging @elastic/apm-ui (Team:apm)

[dgieselaar]

I think there's a bug.

[dgieselaar]

I have some concerns here, mostly around us having to maintain state about CustomPlot in TransactionCharts, which could easily get out of sync. For instance, this happens when I enter a filter into the query bar that gives no results, and then go back:

The axis unit is not updated. Of course we can probably fix this even with this implementation, but it might be better to consider having state only one place (for instance, the consumers of CustomPlot.

[cauemarcondes]

@dgieselaar I've rethought the solution to this problem. I removed the state (a04aaba#diff-da514a7cc4e8210906a22c7f824eaea9L170-L184) and I use asDuration to calculate what will be the unit shown in the y-axis based on the value passed by the CustomPlot. This fixes the issue of this ticket and also the bug you reported.

[formgeist]

We had this up for discussion in our weekly sync, and we agreed to pursue changing the tooltip values to display their individual units appropriate for the value shown.

[formgeist]

LGTM - I think it takes some getting used to, but in the end, I think this is the right approach and most detailed for our users.

[dgieselaar]

@cauemarcondes I'm not sure why in this case durations above 60000ms are not formatted in minutes. The y-axis does use minutes in this case. In this case I would have expected the value above 6000ms to be formatted as minutes.

[cauemarcondes]

I'm not sure why in this case durations above 60000ms are not formatted in minutes. The y-axis does use minutes in this case. In this case I would have expected the value above 6000ms to be formatted as minutes.

Are you sure you pulled the last changes? can you paste the URL, please?

[cauemarcondes]

@elasticmachine merge upstream

[sorenlouv]

I'm not sure this is the right approach. Mainly I don't think we should mix different units (ms, sec, min) in the same visualisation. It's especially confusing when the unit in the tooltip changes for the same metric (eg. 99th is sometimes in minutes and other times in ms).
I agree that the current approach is not great (where the unit is determined from the max value which is always 99th series). Perhaps we should base the unit on the avg series - or the overall average or median value? And then use that unit everywhere (on axis, in tooltips etc). WDYT?

Maybe we can research how other products does this. Are there any white papers around this? Perhaps Elastic Charts have some insights.

[dgieselaar]

@sqren That came up, but IMHO that creates edge cases that feel like they are worse than the problem they are solving (of having different units). Like, a very low avg but a very high 95th percentile becomes unreadable. I'd also consider the implementation cost to be higher because the code has to be aware of an "avg" series vs other series. It also might not necessarily translate well to other charts.

[sorenlouv]

I'd also consider the implementation cost to be higher because the code has to be aware of an "avg" series vs other series. It also might not necessarily translate well to other charts.

This would be solved if we use the overall avg/median, right?

I think what we have today have worked reasonably well (we haven't gotten any issues from users afaik) which is why I'm hesitant to making this change.
From #74830 it looks like the specific problem is that we select minutes as unit instead of seconds.

If we make the following simple change, would that perhaps alleviate the problem for now?

function getDurationUnitKey(max: number): DurationTimeUnit {
-  if (max > toMicroseconds(1, 'hours')) {
+  if (max > toMicroseconds(10, 'hours')) {
    return 'hours';
  }
-  if (max > toMicroseconds(1, 'minutes')) {
+  if (max > toMicroseconds(10, 'minutes')) {
    return 'minutes';
  }
  if (max > toMicroseconds(10, 'seconds')) {
    return 'seconds';
  }
  if (max > toMicroseconds(1, 'milliseconds')) {
    return 'milliseconds';
  }
  return 'microseconds';
}

Explanation: Above change will only use hours as unit if the value is higher than 10 hours, and minutes if the values is larger than 10 minutes

This change will also align the formatting a little more (we still have a preference for milliseconds over microseconds though).
I get that this won't fix extreme cases where 99th is a lot higher than avg. But again: that's always been the case and as long as we don't receive complaints from user's I'd rather leaving it mostly as-is (with minor tweaks).

[dgieselaar]

@sqren I feel like "base the displayed unit on the displayed value" is easy to grasp. All the other solutions have edge cases. The fact that, in some cases (not most), different units will be displayed in the tooltip, doesn't outweigh that cost to me. In general, I feel like every improvement we made to how values are formatted opens up new edge cases. But I've been looking at it from a distance ofc. Anyway, just my 0.02$.

[formgeist]

@sqren I think I agree with your suggestion to change the thresholds for the hours and minutes. We've resolved the initial problem of not updating the y-axis unit labels to fit with the data, this already helps the user in exploring the data appropriately. IMO we got stuck optimizing for a weird edge-case we're seeing in our test data, rather than thinking about the common cases. I wouldn't think to see this large range between average and 95p in a relatively controlled environment. And if that's the case, your suggestion to move the top thresholds for hours and minutes will help as well. I say we move forward with this and see if we get any feedback (we haven't so far).

Thanks,

[dgieselaar]

@sqren actually that's interesting, the table shows a < if the value is lower than 0.1. Maybe that will help here too. But yeah, more logic :) (and the values are sortable in the table which maybe makes it more important to have the same units).

[formgeist]

I think our users are looking for ways to compare the data, especially in the duration chart but also in the tables. I don't find myself looking for a specific number, but rather compare it to other values in the table. Having a "per number unit formatting" will make it cognitively very difficult to keep track and compare. Tbh the examples we're looking at are edge-cases. In the example of this duration chart, a user is moreover looking to compare the percentiles to the average, and keeping to the same unit support this. We already have fixed a problem that enables the user to disable a disruptive 99p to see what the avg. looks like compared to the 95p. If someone wants to investigate other unit improvements, feel free to open an issue, but I don't think this discussion should block this PR for merging.

[cauemarcondes]

@elasticmachine merge upstream

[MartinKolarik]

Adding my two cents here since I'm also frequently affected by this and I see there are several possible solutions discussed. Our usual transaction length is about 1 second, but occasionally, there are spikes up to a few minutes. Currently, that produces completely unhelpful tooltips if there's any such spike in the selected interval. Even for cases where the values are not all displayed as zero, showing them in minutes is way too imprecise.

However, the current solution doesn't work well even if the 99th value is just slightly above 1 minute, because it switches everything to minutes and dealing with numbers like 0.1 minute, 0.2 minute and 1.1 minutes is harder than 3 seconds, 10 seconds, 64 seconds when you are used to everything being in seconds most of the time.

I think switching the units based on currently displayed values makes sense, but ideally, the tooltip would be more useful in the cases I described without manually switching off the 99th percentile line. The solution proposed in #74931 (comment) is something that would help in that regard and something I thought of myself before I found this PR.

Søren is now reviewing this

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 8394800

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
apm	948	+3	945

async chunks size

id	value	diff	baseline
apm	4.7MB	-1.8KB	4.7MB

History

• 💚 Build #70855 succeeded 08b307c
• 💔 Build #70823 failed a904331ef6f3f546979164b16e27e17b537e7abf
• 💔 Build #70481 failed c3b456a9c4a8b9bcbc20a371024dd32b705d4bbd
• 💔 Build #70469 failed be5e8b3e363a1f8ef7195d3579c9d548d0f62531
• 💚 Build #69908 succeeded 4026b33

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[dgieselaar]

Works well in all browsers 👍