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

Add statistics to query_range and instant_query API. #1615

Merged
merged 16 commits into from
Feb 4, 2020

Conversation

cyriltovena
Copy link
Contributor

@cyriltovena cyriltovena commented Jan 30, 2020

This introduces a new statistics property to the query_range, instant_query API and also the old query API.

The frontend also honor and merge those statistics although there is two small caveats:

  • it doesn't account for time spent in the frontend (planning to improve this in another PR), only queriers exec time is computed.
  • result_cache discard statistics (I don't think we want to account for the result_cache anyways)

I think it would be useful may be to add more frontend (eg. amount of split, result cache hit) and chunk/index cache statistics in the long term. But this seems like a good start.

The statistics property contains informations about the execution of the query such as processed lines/bytes per seconds from storage and ingesters.

This is a draft PR as I have to rebase and add API documentation for this.

PS: Unfortunately the Cortex result cache used for metrics requires protof response type and since Cortex wasn't using the same gogoproto version I had to downgrade our version to (1.2.1) until we are in sync, there is only minor bugfix that we don't use in 1.3.0 and I've also opened a PR in Cortex to make the change. See cortexproject/cortex#2055

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena changed the title Add statistics to query_range API. Add statistics to query_range and instant_query API. Jan 30, 2020
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

This is looking great! I just have one question about how it's exposed to the user. I remember some internal conversations about matching what Prometheus does.

pkg/loghttp/query.go Outdated Show resolved Hide resolved
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@3ab590b). Click here to learn what that means.
The diff coverage is 86.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1615   +/-   ##
=========================================
  Coverage          ?   61.69%           
=========================================
  Files             ?      109           
  Lines             ?     8260           
  Branches          ?        0           
=========================================
  Hits              ?     5096           
  Misses            ?     2770           
  Partials          ?      394
Impacted Files Coverage Δ
pkg/iter/iterator.go 60.48% <0%> (ø)
pkg/loki/modules.go 11.29% <0%> (ø)
pkg/loghttp/query.go 47.36% <0%> (ø)
pkg/querier/http.go 7.06% <0%> (ø)
pkg/logql/marshal/marshal.go 75.86% <100%> (ø)
pkg/querier/queryrange/prometheus.go 89.18% <100%> (ø)
pkg/logql/marshal/legacy/marshal.go 63.63% <100%> (ø)
pkg/querier/queryrange/codec.go 95.68% <100%> (ø)
pkg/logql/stats/context.go 92.23% <100%> (ø)
pkg/querier/querier.go 70.06% <100%> (ø)
... and 7 more

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena cyriltovena marked this pull request as ready for review February 3, 2020 22:40
@cyriltovena
Copy link
Contributor Author

This is ready for reviews.

/cc @owen-d

docs/api.md Show resolved Hide resolved
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM other than two indentation problems

pkg/logql/stats/stats.proto Outdated Show resolved Hide resolved
pkg/logql/stats/stats.proto Outdated Show resolved Hide resolved
pkg/logql/engine.go Show resolved Hide resolved
pkg/logql/stats/stats.proto Outdated Show resolved Hide resolved
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Copy link
Member

@rfratto rfratto left a comment

Choose a reason for hiding this comment

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

LGTM 🎉

@cyriltovena cyriltovena merged commit 93eb4ad into grafana:master Feb 4, 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.

4 participants