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 ingester results for series query #2264

Merged
merged 1 commit into from
Jul 10, 2020

Conversation

adityacs
Copy link
Contributor

What this PR does / why we need it:
While querying series API, ingester returns the result even when the request time range is outside of chunk time range. This PR fixes this issue.

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2020

Codecov Report

Merging #2264 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2264      +/-   ##
==========================================
+ Coverage   62.44%   62.47%   +0.03%     
==========================================
  Files         158      158              
  Lines       12762    12769       +7     
==========================================
+ Hits         7969     7978       +9     
  Misses       4177     4177              
+ Partials      616      614       -2     
Impacted Files Coverage Δ
pkg/ingester/instance.go 63.30% <100.00%> (+1.06%) ⬆️
pkg/querier/queryrange/downstreamer.go 95.87% <0.00%> (-2.07%) ⬇️
pkg/logql/evaluator.go 92.54% <0.00%> (+0.40%) ⬆️
pkg/promtail/targets/tailer.go 78.40% <0.00%> (+2.27%) ⬆️

@slim-bean
Copy link
Collaborator

The other PR I have open is gonna screw this one up a bit, and also looks like I introduce another case where you could hit this same bug, I'd like to get that merged first if we could.

Also we should see if we can get some tests on this, that would be good.

@cyriltovena
Copy link
Contributor

FYI The other PR has been merged, you can merge upstream.

@adityacs
Copy link
Contributor Author

I think I created this PR just before @slim-bean's PR got merged. I will rebase this.

@pull-request-size pull-request-size bot added size/L and removed size/S labels Jun 30, 2020
@adityacs adityacs force-pushed the series_ingester_results branch 2 times, most recently from b3936f4 to d16b415 Compare June 30, 2020 09:04
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

lgtm

@adityacs
Copy link
Contributor Author

adityacs commented Jul 3, 2020

This should not be merged. Looks like this change will induce one more data race. We have to fix this first #2265

@adityacs
Copy link
Contributor Author

This should be good to merge now. No change required on this PR.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

Great work !

@cyriltovena cyriltovena merged commit b82d2e8 into grafana:master Jul 10, 2020
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.

5 participants