-
Notifications
You must be signed in to change notification settings - Fork 14
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
Shard query splitting: Split queries by stream shards #715
Conversation
should this be in draft for now? |
100%. Just wanted the build for publishing. |
4706de5
to
3a5df48
Compare
@grafana/observability-logs If you want to start the review while I work on the coverage, that'd be great. I don't plan to add any significant change to the code for now. |
Coverage added 😤 |
@@ -0,0 +1,1403 @@ | |||
import { DataFrame, DataFrameType, DataQueryResponse, Field, FieldType, QueryResultMetaStat } from '@grafana/data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,281 @@ | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,194 @@ | |||
import { of } from 'rxjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -0,0 +1,264 @@ | |||
import { Observable, Subscriber, Subscription } from 'rxjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
69da7ea
to
c0df8ca
Compare
e14bb19
to
6c34669
Compare
Definitely. What I wanted to try was to create the initial frame with fields with empty values, and see if that gets rid of that behavior. I did a quick test, but it will require an update to the merging code and some extra complexity, so I'd like to follow that up independently. And/or check the viz to see what is it that triggers the display of No Data. |
From what I remember the streaming (LoadingState) state expects dataframes, I think setting the initial state as LoadingState.Loading fixes 90% of this without any other refactor |
Fantastic, I'll give it a try. The reason why we used streaming with splitting was so that the panel updates the viz as new data appears, but we can start with loading and change to streaming easily. |
Yeah I think it should be "loading" state, until we get a non-empty response. We shouldn't update the "series" prop until we get a non-empty response, or the request is done. But easier said then done :P if it's too much work we can punt on it as this is under a ff, but this is the only real issue I'm seeing right now in my testing. A good way to see how big of a UX change this is, is by enabling auto-refresh on this branch vs main |
This reverts commit c6c2cd0.
ed238fb
to
bbe917c
Compare
Query splitting by stream shards.
Query splitting was introduced in Loki to optimize querying for long intervals and high volume of data, dividing a big request into smaller sub-requests, combining and displaying the results as they arrive.
This approach, inspired by the time-based query splitting, takes advantage of the stream_shard internal label, representing how data is spread into different sources that can be queried individually.
The main entry point of this module is runShardSplitQuery(), which prepares the query for execution and passes it to splitQueriesByStreamShard() to begin the querying loop.
splitQueriesByStreamShard() has the following structure:
Key features
/values
call including the selected service.__stream_shard__=~"shardn1|shardn2|...|shardn"
.Important
Requires the
exploreLogsShardSplitting
feature flag enabledCloses #690