-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Logcli: automatically batch requests #2482
Conversation
fixing up how the MockQuerier works.
@@ -119,7 +130,22 @@ func (c *Client) Series(matchers []string, from, through time.Time, quiet bool) | |||
return &seriesResponse, nil | |||
} | |||
|
|||
func (c *Client) doQuery(path string, query string, quiet bool) (*loghttp.QueryResponse, error) { | |||
// LiveTailQueryConn uses /api/prom/tail to set up a websocket connection and returns it | |||
func (c *DefaultClient) LiveTailQueryConn(queryStr string, delayFor int, limit int, from int64, quiet bool) (*websocket.Conn, error) { |
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.
I only moved this to keep it in the same order as the interface and put it before private methods
Codecov Report
@@ Coverage Diff @@
## master #2482 +/- ##
==========================================
+ Coverage 62.91% 63.19% +0.27%
==========================================
Files 162 162
Lines 13998 14068 +70
==========================================
+ Hits 8807 8890 +83
+ Misses 4502 4476 -26
- Partials 689 702 +13
|
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.
Edited files
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
Co-authored-by: Diana Payton <52059945+oddlittlebird@users.noreply.github.com>
break | ||
} | ||
if len(lastEntry) >= q.BatchSize { | ||
log.Fatalf("Invalid batch size %v, the next query will have %v overlapping entries "+ |
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.
I think it'd be better to just return up to limit
, even if it doesn't consume all entries with the same timestamp. Not a blocker though.
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.
Batches are printed as they are received so in this case we would print the last batch but then fail before going after the next (because it would effectively have exactly the same entries as the last batch)
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.
What this error is saying is basically you queries with a batch size of 1000 and all 1000 entries have the same timestamp so you have to increase your batch size until that isn't true.
With a batch size default of 1000 I hope it's pretty unlikely people will ever run into this.
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.
I think whoever finds themselves hitting this error should open a pull request to make this better :)
one nit, but ltgm |
Loki has a server side limit configured for the maximum number of log lines returned in a single query.
This can be cumbersome at the command line as often with logcli we pipe the output of the result into other tooling.
This PR adds automatic support for batching requests to Loki handled entirely on the client side.
You can now specify a
--limit
of any size and in the background logcli will make the request in batches sized on--batch
which defaults at 1000.Some notes:
Currently every request has separate information printed including stats, so you will see interruptions in the output on the console (these are sent to
stderr
however so they do not affect the output.Use the
--quiet
flag to hide this information.In the future we may want to consider handling this output differently, similarly
--stats
will be printed with each request.