Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explore: Support mixed data sources for supplementary query #63036

Merged
merged 64 commits into from Mar 7, 2023

Conversation

ifrost
Copy link
Contributor

@ifrost ifrost commented Feb 7, 2023

This PR adds support for showing multiple supplementary query results (logs volume and samples) when a mixed data source is used.

We have discussed a few approaches with @Elfo404 and @gelicia and decided to go with something that will break the current behavior but will allow more flexibility in the future:

  • Log volume is not aggregated automatically. Instead, each data source passed multiple data frames referencing the source query in the result. Each log volume histogram is displayed separately per query.
  • Fallback logs volume is now calculated only when needed and per each query separately.
  • The visualization gets all data frames for all log volumes and can decide how to render them. This allows us to add much better log volume aggregation in the future if needed. For example, we can aggregate volumes by data source, all of them into one, or even allow the user to decide what should be aggregated and what now.

On top of the changes, the PR adds support for Log Samples with all results displayed in a single panel.

How to test it? Use various combinations of log queries, use Loki/Elastic for full range histograms, and others, like Test Datasource for results based histogram.

Relates to #61546

To actually mark #61546 as fixed we still need to adjust y/x axis

…-data-source-and-supplementary-queries

# Conflicts:
#	public/app/features/explore/Graph/ExploreGraph.tsx
…ueries

# Conflicts:
#	packages/grafana-data/src/types/logs.ts
#	public/app/features/explore/LogsVolumePanel.tsx
#	public/app/features/explore/state/query.ts
#	public/app/features/explore/state/supplementaryQueries.ts
@ifrost ifrost added this to the 9.5.0 milestone Feb 8, 2023
@ifrost ifrost added the no-backport Skip backport of PR label Feb 8, 2023
@gelicia
Copy link
Contributor

gelicia commented Feb 24, 2023

Couple more odd behaviors

Scenario 1

  1. Start on default explore. Select elasticsearch. Select logs, and go back as many days needed to find logs and get a log volume chart
  2. Click Mixed. Add query. Another elasticsearch query opens and shows a time series chart as expected.
  3. Change query B to Loki. Time series chart remains (not expected).
  4. Running the query by hitting the button does show the log volume for each and removes the time series chart.

Scenario 2

  1. Start on default explore. Select Loki, select a label filter. See logs and logs volume
  2. Click Mixed. Add query. Change to elasticsearch. Empty elasticsearch query displays. Collapsing/expanding query shows query options.

@ifrost ifrost linked an issue Feb 26, 2023 that may be closed by this pull request
@ifrost
Copy link
Contributor Author

ifrost commented Feb 26, 2023

Couple more odd behaviors (...)

I see that both are happening in main and I don't think they are related to changes in this PR. I've created separate issues to address them: #63780 and #63781

Copy link
Member

@Elfo404 Elfo404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review, took this for a ride and seems to work as expected, great job!

i need to take a better look at a few things, in the meantime i left some minor suggestions

.betterer.results Show resolved Hide resolved
public/app/core/logsModel.ts Show resolved Hide resolved
public/app/features/explore/LogsVolumePanel.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogsVolumePanelList.test.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogsVolumePanelList.tsx Outdated Show resolved Hide resolved
public/app/features/explore/LogsVolumePanelList.tsx Outdated Show resolved Hide resolved
Copy link
Member

@Elfo404 Elfo404 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went through this again and i don't have much to add on top of what i already wrote, seems to work as expected and the code is super easy to follow 🙌

i just left a couple of more nits, but that's also something we can iterate on in the future. LGTM and nice stuff again!

packages/grafana-data/src/types/logs.ts Outdated Show resolved Hide resolved
packages/grafana-data/src/types/logs.ts Outdated Show resolved Hide resolved
public/app/features/explore/LogsVolumePanelList.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ifrost I am wondering, what were the other options for how to handle logs volume for multiple queries and what were the arguments that this approach win. I am just curious, because everywhere else in Explore/Grafana, we show results of multiple queries in 1 visualisation (e.g. if 2 queries return table results, we show it in 1 table). But this approach creates a new exception/new pattern. Have you considered just merging all Limited logs volume in 1 visualisation and all Full range logs volume into 1?

@ifrost
Copy link
Contributor Author

ifrost commented Mar 6, 2023

@ifrost I am wondering, what were the other options for how to handle logs volume for multiple queries and what were the arguments that this approach win. I am just curious, because everywhere else in Explore/Grafana, we show results of multiple queries in 1 visualisation (e.g. if 2 queries return table results, we show it in 1 table). But this approach creates a new exception/new pattern. Have you considered just merging all Limited logs volume in 1 visualisation and all Full range logs volume into 1?

@ivanahuckova

First we tried just to merge everything. It didn't work out because merging Limited and Full Range volumes causes confusion. Limited bars have more narrow range so they are included only in some of Full Rage bars.

So then we tried splitting it into 2 log volumes - Limited and Full Range. It turned out that merging Full Range volume didn't work nicely when two different data sources are used, for example Loki and Elastic. Start and end time in some cases are slightly off, for example the start/end of the range is included or not. That caused weird issues at the edge of the graph. Also in practice we cannot guarantee 100% exact same bucketing which causes misalignment of bars and graph becomes unreadible.

There was one more issue with Limited log volume. Originally we based it on all lines in logs panel. That was also confusing because values were duplicated/repeated, for example same log line was included in Full Range and Limited log volume results.

Soooo we tried splitting it by data source. Within the data source it should be always possible to merge graphs together, but that just felt strange. Let's say you have 3 queries, 1 for Loki and 2 for Elastic. Log volume would show two graphs - one for Loki and one for Elastic, and we doubted that merging two queries just because they belong to the same data source should be merged.

So we thought that there seem to be no right answer on how to aggregate so maybe we should not aggregate them and display to the user something that is the simplest possible thing - one volume per query. In most cases if the user needs to see an aggregated histogram for multiple queries in the same data source it should be possible to write a single query for it. It's not always possible, but with the current approach we can decide about how to move forward based on user feedback.

Now I'm wondering that it probably might be a good idea to add some tracking to get stats about log volume, as in how many different log volumes are displayed and which data sources were used. WDYT?

Copy link
Member

@ivanahuckova ivanahuckova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ifrost thank you for providing context! The implementation looks good. The only thing that slightly worries me is that in the future, we plan to remove mixed data sources from data source selector. And it is going to be the default state. Which would mean, that if you select 2 loki data sources and run log query, the UI is going to display 2 histograms even though showing 1 histogram would be better and more UX consistent. So to support VERY RARE edge case (e.g. having 1 log query from ES and 1 from Loki), we are worsening UI for less rare case - having 2 loki logs queries. But we can improve/re-consider this when we introduce mixed data sources as default case.

I am wondering, could we create issue for it so we remember to look into it when mixed data sources will be default? Or what are your thoughts? 🙂

@ifrost
Copy link
Contributor Author

ifrost commented Mar 7, 2023

The only thing that slightly worries me is that in the future, we plan to remove mixed data sources from data source selector. And it is going to be the default state. Which would mean, that if you select 2 loki data sources and run log query, the UI is going to display 2 histograms even though showing 1 histogram would be better and more UX consistent.

@ivana This PR already changes that behavior, it's not only for mixed data sources, example:

Screenshot 2023-03-07 at 12 05 41

Mixed data source just uses the same logic. Do you think we should aggregate volumes? If so, what should be the logic based on: data source name or data source type? Or something else? Do you think it's a blocker for this PR?

@ivanahuckova
Copy link
Member

Do you think it's a blocker for this PR?

Definitely not a blocker as most of the users run just 1 Loki query.

I'd personally aggregate volumes based on data source names, so we can specify the name in the same way as it is now, but we can also bring this up in UX feedback session. So as long as we make an issue and effort to consider if we should aggregate data based on ds name/type, then LGTM. 🙂

@ifrost
Copy link
Contributor Author

ifrost commented Mar 7, 2023

So as long as we make an issue and effort to consider if we should aggregate data based on ds name/type, then LGTM.

Coolio. I've added a task to #54533. Sounds good? (cc @diegoadams @radiorental)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explore: Support log volume histogram for mixed data sources
5 participants