[BUGFIX] Logstable: all quries results must be included in the logs table

[shahrokni]

Description 🖊️

Working on perses/perses#3786 I found a bug which may also lead to other bugs related to transform. Let's see!

Logs table should include the result of all queries. It seems the buggy version only includes the query result at index 0.

Checklist

• Pull request has a descriptive title and context useful to a reviewer.
• Pull request title follows the [<catalog_entry>] <commit message> naming convention using one of the
  following catalog_entry values: FEATURE, ENHANCEMENT, BUGFIX, BREAKINGCHANGE, DOC,IGNORE.
• All commits have DCO signoffs.

UI Changes

• Changes that impact the UI include screenshots and/or screencasts of the relevant changes.
• Code follows the UI guidelines.

[jgbernalp]

I think this makes sense. We are now merging unordered logs, but I think sorting is done when the table renders. Can we check that?

WDYT about this @rickardsjp @rafi-ruetcse17 @abelyakin?

[shahrokni]

I think this makes sense. We are now merging unordered logs, but I think sorting is done when the table renders. Can we check that?

WDYT about this @rickardsjp @rafi-ruetcse17 @abelyakin?

I see no specific mechanism for sorting or ordering mechanism. Technically, the input is rendered as it is.
The results of individual queries pile up.

const logs = [...q1_results, ...q2_results];

      <Virtuoso
        style={{ height: '100%', flexGrow: 1 }}
        initialItemCount={spec.showAll ? logs.length : undefined}
        totalCount={logs.length}
        itemContent={renderLogRow}
      />

/* --- */

  const renderLogRow = (index: number) => {
    const log = logs[index];
    if (!log) return null;

    return (
      <LogRow
        isExpandable={spec.enableDetails}
        log={log}
        index={index}
        isExpanded={expandedRows.has(index)}
        onToggle={onToggleExpand}
        allowWrap={spec.allowWrap}
        showTime={spec.showTime}
        isSelected={selectedRows.has(index)}
        onSelect={handleRowSelect}
      />
    );
  };

[shahrokni]

@jgbernalp
Now the merged results are ordered according to the timestamp

[jgbernalp]

LGTM, I'll let others chime in before merging.

[rickardsjp]

Great find @shahrokni and thanks for tagging me @jgbernalp. I like it. I think we should eventually make the sorting order configurable (e.g. "flip results order" button) but that's out of scope for a bugfix PR.

[rickardsjp]

If a user has more than two queries, we add the log entries multiple times, which may be bad for performance (number of copy operations grows quadratically). There is flatMap(), which I understand would make this step at least a linear operation. I've tested it locally and it also works.

Suggested change

	const logs = queryResults
	.reduce<LogEntry[]>((acc, log) => {
	acc = acc.concat(log?.data.logs?.entries ?? []);
	return acc;
	}, [])
	const logs = queryResults
	.flatMap(result => result?.data.logs?.entries ?? [])

I guess that's mostly a consideration for log views with multiple queries with large results which are unlikely, but worth noting.

[rickardsjp]

This is good (reversing the timestamps) but only tests sorting and does not test whether results from multiple queries are included.

[abelyakin]

Lgtm.
Good catch! Logs indeed sorted per query.

[rickardsjp]

picking one last nit:

[rickardsjp]

Don't think we needed a new attribute here. From the docs:

Using data-testid attributes do not resemble how your software is used and should be avoided if possible.

Couldn't we have done something like

const items = items.querySelectorAll('[data-log-index]');

in the test instead?

But I think we can refactor this once we add a button for inverting the result order.