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

fix store query bug when running loki in single binary mode with boltdb-shipper #2655

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
When running loki with boltdb-shipper in single binary mode and doing queries not touching the recent data I found that store was not being queried. The problem appears to be here which considers end time of query instead of time.Now() while calculating adjustedEnd of query. If you do a query for yesterday with 1h interval and IngesterQueryStoreMaxLookback is set to 2h then the adjustedEnd would be set to older value than start time of query hence the condition here would be true and result in not querying the store.

I have refactored the code to do all the calculations in a separate function which has following properties:

  1. IngesterQueryStoreMaxLookback would take precedence over QueryIngestersWithin config.
  2. If query covers both ingersters and queries, IngesterQueryStoreMaxLookback would do separate non overlapping queries to stores and ingesters while QueryIngestersWithin would do same queries to both which is same as how it was before.

Checklist

  • Tests updated

@codecov-commenter
Copy link

Codecov Report

Merging #2655 into master will increase coverage by 0.24%.
The diff coverage is 73.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2655      +/-   ##
==========================================
+ Coverage   61.22%   61.46%   +0.24%     
==========================================
  Files         172      172              
  Lines       13360    13380      +20     
==========================================
+ Hits         8179     8224      +45     
+ Misses       4431     4410      -21     
+ Partials      750      746       -4     
Impacted Files Coverage Δ
pkg/querier/querier.go 70.71% <73.84%> (+11.35%) ⬆️
pkg/querier/queryrange/downstreamer.go 95.29% <0.00%> (-2.36%) ⬇️
pkg/ingester/transfer.go 58.91% <0.00%> (-1.56%) ⬇️
pkg/logql/evaluator.go 92.81% <0.00%> (+0.42%) ⬆️
pkg/promtail/targets/file/tailer.go 77.52% <0.00%> (+8.98%) ⬆️

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM!

@sandeepsukhani sandeepsukhani merged commit 6053cdb into grafana:master Sep 25, 2020
Comment on lines +177 to +191
// query ingester for whole duration.
if ingesterMLB == -1 {
i := &interval{
start: queryStart,
end: queryEnd,
}

if limitQueryInterval {
// query only ingesters.
return i, nil
}

// query both stores and ingesters without limiting the query interval.
return i, i
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why query ingester for whole duration ?

Copy link
Contributor

Choose a reason for hiding this comment

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

image

The logic here is inconsistent with the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to support the rare usecase where you run Loki with multiple single binaries and each of them storing the data on local filesystem without any sharing.

Comment on lines +203 to +209
if !limitQueryInterval {
i := &interval{
start: queryStart,
end: queryEnd,
}
return i, i
}
Copy link
Contributor

@lzh-lab lzh-lab Dec 6, 2021

Choose a reason for hiding this comment

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

The microservices deployment mode with object store will run here no matter if there is an overlap.

ingester:
        query_store_max_look_back_period: 0  
querier:
        query_ingesters_within: 2h

This case will also query ingester for whole duration.

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