[APM] Profiling flamegraph text uses node id instead of label

[trentm]

Versions
Kibana: master

The issue:

Those box labels in the flamegraph are using .id values rather than .label values. The tooltips are getting the correct values. When it was working it looked like this:

Some details:

In elastic/kibana#91818, @dgieselaar added a "Profiling" section to the APM app with a flamegraph, using an icicle chart from @elastic/charts. At the time of that commit Kibana master was using @elastic/[email protected].

Kibana commit elastic/kibana@2bfba7b4602 (#94505) broke it. That change updated to @elastic/[email protected].

Playing directly with charts version, v25.1.1 works, but v25.2.0 fails. That means the breaking change is in v25.1.1...v25.2.0 From those commits, I assume it is #1041 that changed behaviour to break this flamegraph in Kibana. I don't know if it is a bug in elastic-charts or an incompatible change that Kibana's code need to adjust to. Both of these comments from that PR seem possibly relevant:

• as the drilldown logic is different, the recently added groupByRollup and tree.ts additions for drilldown became unnecessary, they got removed
• optimizes the clipped text render path for icicle/flame for speed, by adding a specialized render function with reduced API and function call count, and by avoiding multirow text fill calculation in viewmodel.ts

Here is Kibana's code creating this chart: https://github.com/elastic/kibana/blob/9d9dfe4bbf6b37243b509a99375067a8781e32a7/x-pack/plugins/apm/public/components/app/service_profiling/service_profiling_flamegraph.tsx#L123-L337

/cc @dgieselaar @monfera @nickofthyme

[elasticmachine]

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

[nickofthyme]

Thanks for the bisect @trentm. @monfera this is likely from changes in a9648a4, do you have any idea what could have caused this apparentt regression?

[sorenlouv]

@monfera Should I transfer this issue to elastic-charts repo?

[monfera]

Hi @trentm @sqren thanks for the report, and thanks @nickofthyme for pinging me, the github notifications got swallowed somewhere.

In the grand scheme of things, there can be three different pieces of text or alphanumerical values:

• an identifier, which doesn't show up anywhere, but is expected to be unique
• the text that goes into the boxes (no expectation of uniqueness)
• the text that shows up on the tooltip (no expectation of uniqueness)

It's not currently super clear which image of the two is considered good vs bad, can you please clarify? Also, I just see different texts, and don't have the data, so it'd be best to tag each image with where it appears to get the texts from. One of the shots has a tooltip and the other doesn't, so I'm guessing that what the reported problem is that instead of the id, now the label is used now in the text box, which is also used for the tooltip. But the title of the issue seems to be the exact reverse of that, so I'd like to ask clarification for that.

As the change was a while ago and I wasn't aware that it went into Kibana then, some time passed, I can only guess it was an intentional fix. The bars can be of arbitrary width, so even now I think there's utility to showing the same text in the tooltip as what's shown in the box. Is it something that we agree on? Other flamegraphs are like that.

If this is the case, maybe we can pose it as a request eg. in elastic-charts that

• there be extra information in the tooltip besides the box text (eg. 2 rows in the tooltip)
• (maybe) the box text should be configurable for optionally disabling it for some use cases

Either way, an elastic-charts issue is a good idea, because, at a minimum, my understanding is that you need two different texts that need to show up (box and tooltip) and since we've no guarantees of the uniqueness of either, it needs the third one (unique identifier).

[monfera]

Btw. I'm glad to make the gh issue in elastic-charts, already super happy with the quick adoption of this chart type and the detailed report 👍

[trentm]

It's not currently super clear which image of the two is considered good vs bad, can you please clarify?

Hopefully this clarifies:

The two screenshots above are for the same data. The first (bad) case is using @elastic/[email protected]. The second (good) case is using @elastic/[email protected].

there can be three different pieces of text or alphanumerical values:

* an identifier, which doesn't show up anywhere, but is expected to be unique
* the text that goes into the boxes (no expectation of uniqueness)
* the text that shows up on the tooltip (no expectation of uniqueness)

In the example data above, for the box just below "root":

1. identifier is b2d9990eeb566151
2. box text is processTicksAndRejections:69
3. tooltip text is internal/process/task_queues.js/processTicketsAndRejections:69 26 ms (0%)

It seems to me (without understanding the code) that the chart is using identifier for the box text rather than calling the registered nodeLabel() method (https://github.com/elastic/kibana/blob/9d9dfe4bbf6b37243b509a99375067a8781e32a7/x-pack/plugins/apm/public/components/app/service_profiling/service_profiling_flamegraph.tsx#L258-L263).

The bars can be of arbitrary width, so even now I think there's utility to showing the same text in the tooltip as what's shown in the box. Is it something that we agree on? Other flamegraphs are like that.

If this is the case, maybe we can pose it as a request eg. in elastic-charts that

* there be extra information in the tooltip besides the box text (eg. 2 rows in the tooltip)
* ...

For APM UI I believe we'll want to have the ability to specify different string values for the "box text" and the "tooltip text" (we'll show more data in the tooltip). I agree that typically the tooltip text value will include the box text (plus more). I'm not one of the designers, nor even one of the UI engineers. I don't think we've settled on or discussed what format the tooltip should be. Eventually I think it would be nice to be able to have a tooltip with more structure than just a string (styling, multi-line).

[sorenlouv]

Transferred to elastic-charts repo.