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

Logcli: automatically batch requests #2482

Merged
merged 17 commits into from
Aug 10, 2020
Merged

Logcli: automatically batch requests #2482

merged 17 commits into from
Aug 10, 2020

Conversation

slim-bean
Copy link
Collaborator

@slim-bean slim-bean commented Aug 9, 2020

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.

@@ -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) {
Copy link
Collaborator Author

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-commenter
Copy link

codecov-commenter commented Aug 9, 2020

Codecov Report

Merging #2482 into master will increase coverage by 0.27%.
The diff coverage is 51.75%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pkg/logcli/client/client.go 5.83% <0.00%> (-0.10%) ⬇️
pkg/logcli/query/tail.go 0.00% <0.00%> (ø)
pkg/logcli/output/raw.go 50.00% <33.33%> (-50.00%) ⬇️
pkg/logql/test_utils.go 80.79% <54.54%> (-2.66%) ⬇️
pkg/logcli/query/query.go 38.65% <62.12%> (+38.65%) ⬆️
pkg/logcli/output/default.go 75.00% <66.66%> (-3.58%) ⬇️
pkg/logcli/output/jsonl.go 80.00% <100.00%> (ø)
pkg/logcli/output/output.go 92.59% <100.00%> (+0.92%) ⬆️
pkg/logql/evaluator.go 92.47% <0.00%> (-0.41%) ⬇️
pkg/promtail/targets/file/filetarget.go 69.64% <0.00%> (+1.78%) ⬆️
... and 2 more

Copy link
Contributor

@oddlittlebird oddlittlebird left a comment

Choose a reason for hiding this comment

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

Edited files

docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
docs/sources/getting-started/logcli.md Outdated Show resolved Hide resolved
owen-d and others added 8 commits August 10, 2020 16:11
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 "+
Copy link
Member

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.

Copy link
Collaborator Author

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)

Copy link
Collaborator Author

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.

Copy link
Collaborator Author

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 :)

@owen-d
Copy link
Member

owen-d commented Aug 10, 2020

one nit, but ltgm

@slim-bean slim-bean merged commit 80693ca into master Aug 10, 2020
@slim-bean slim-bean deleted the logcli-batch branch August 10, 2020 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants