Skip to content

Query Frontend Refactor: Metrics Query Range - #3584

Merged
joe-elliott merged 19 commits into
grafana:mainfrom
joe-elliott:refactor-metrics-2
Apr 19, 2024
Merged

Query Frontend Refactor: Metrics Query Range#3584
joe-elliott merged 19 commits into
grafana:mainfrom
joe-elliott:refactor-metrics-2

Conversation

@joe-elliott

@joe-elliott joe-elliott commented Apr 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Collaborator

What this PR does:
Moves the final frontend endpoint (metrics query range) over to the new streaming pipeline. Note that the streaming is done in a naive fashion. We simply return whatever is available every 500ms. After this is integrated into a frontend we will revisit and determine what a good streaming pattern will be.

Open Questions

  • Should we add a new SLO config and SLO metric explicitly for metrics? Currently it piggybacks on search.
  • Should we reorg the code layout to be around pipeline? i.e. should we bring the combiner/sharder/handlers all together in modules? They are starting to have dependencies that would be easier to see if they were co-located.

Other changes:

  • Refactors the pipeline to return an interface instead of *http.Response. The interface allows additional data to accompany the returned result.
    • Adds the ability for a request to add a value into the context. This value is then returned with its associated response. This is used to return a per job sampling rate to the combiner.
  • Drops prom compatibility support
  • Adds support in the CLI for the new endpoints
  • Updates the local docker-compose example to support metrics queries
  • Adds guard code in the frontend to prevent invalid config options
  • Removes a number of unused files such as the no-op combiner and the old multitenant sharder
  • Added total jobs and completed jobs to the metrics query range response
  • Excludes goconst linter from _test.go files.

Which issue(s) this PR fixes:
Fixes #3413

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
@joe-elliott
things technically compile!
e51fbd5 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Succeed with no data
a0f8b89 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
remove prom compat
7dc7789 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Add tests for the metrics pipeline and fix
c2851dd 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
cleanup
506b649 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
restore sorting
ab08e9b 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
removed unreferenced files
9dfc168 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
migrate to response interface
ff61860 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
Sampling rate!
1a7f663 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
remove no-op
c724007 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
wip: add cli
961ffd8 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
QueryRange -> MetricsQueryRange
61acd0e 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
changelog and other cleanup
5bd1ea5 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
lint
7bdb436 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
lint + guardcode
c7a4b21 
Signed-off-by: Joe Elliott <number101010@gmail.com>
mdisibio
mdisibio reviewed Apr 17, 2024
View reviewed changes
Comment thread pkg/api/http.go Outdated
Comment thread modules/frontend/metrics_query_range_sharder.go
Comment thread modules/frontend/combiner/metrics_query_range.go Outdated
if samplingRate != nil {
fRate := samplingRate.(float64)

if fRate != 0.0 {

@mdisibio mdisibio Apr 17, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: can skip this when rate == 1.0. Maybe switch to fRate < 1.0 ?

@joe-elliott joe-elliott Apr 17, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

the 0.0 check is just a sanity check to prevent panics. fRate < 1.0 && fRate > 0.0. would there ever be a time we deflated metrics with the rate?

@mdisibio mdisibio Apr 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, the sampling rate will always be between 0 and 1 due to the check in the sharder: https://github.com/grafana/tempo/pull/3584/files#diff-85974350e62b77bee6b5d88d537e3946839ccb1bba5f6dc89450a1c5489b5d4fR364

@joe-elliott joe-elliott Apr 19, 2024

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good catch. updated to the suggested condition
Comment thread modules/frontend/frontend.go
@joe-elliott
change the way we're ignoring tests
763a51d 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
clean up url parsing
4f14b6c 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
fixed url parsing and add test
94c739d 
Signed-off-by: Joe Elliott <number101010@gmail.com>
@joe-elliott
joe-elliott requested a review from mdisibio April 18, 2024 14:12
@joe-elliott
adjust sanity check
30fa869 
Signed-off-by: Joe Elliott <number101010@gmail.com>
mdisibio
mdisibio approved these changes Apr 19, 2024
View reviewed changes

@mdisibio mdisibio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM and nice work. I look forward to glorious streaming metrics panels!
@mdisibio

mdisibio commented Apr 19, 2024

Copy link
Copy Markdown
Contributor

Should we add a new SLO config and SLO metric explicitly for metrics? Currently it piggybacks on search.

I think in the future yes, but not required for this PR. Even after we make the next round of changes that brings the metrics read path much closer to the search read path, I still expect the minimum throughout and latency to be different.

Should we reorg the code layout to be around pipeline? i.e. should we bring the combiner/sharder/handlers all together in modules? They are starting to have dependencies that would be easier to see if they were co-located.

Yeah open to another reshuffle if it makes sense.
@joe-elliott
joe-elliott merged commit c59bb01 into grafana:main Apr 19, 2024
@electron0zero electron0zero mentioned this pull request May 8, 2024
Merged
3 tasks
electron0zero added a commit that referenced this pull request May 9, 2024
@electron0zero
Split metrics slo out from search slo (#3659)
a853852 
In #3584 we decided to count metrics queries as part of search slo. 

split metrics slo out of search because metrics are still experimental and should not share the SLO with search.
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
@joe-elliott
Query Frontend Refactor: Metrics Query Range (grafana#3584)
ef35787 
* things technically compile!

Signed-off-by: Joe Elliott <number101010@gmail.com>

* Succeed with no data

Signed-off-by: Joe Elliott <number101010@gmail.com>

* remove prom compat

Signed-off-by: Joe Elliott <number101010@gmail.com>

* Add tests for the metrics pipeline and fix

Signed-off-by: Joe Elliott <number101010@gmail.com>

* cleanup

Signed-off-by: Joe Elliott <number101010@gmail.com>

* restore sorting

Signed-off-by: Joe Elliott <number101010@gmail.com>

* removed unreferenced files

Signed-off-by: Joe Elliott <number101010@gmail.com>

* migrate to response interface

Signed-off-by: Joe Elliott <number101010@gmail.com>

* Sampling rate!

Signed-off-by: Joe Elliott <number101010@gmail.com>

* remove no-op

Signed-off-by: Joe Elliott <number101010@gmail.com>

* wip: add cli

Signed-off-by: Joe Elliott <number101010@gmail.com>

* QueryRange -> MetricsQueryRange

Signed-off-by: Joe Elliott <number101010@gmail.com>

* changelog and other cleanup

Signed-off-by: Joe Elliott <number101010@gmail.com>

* lint

Signed-off-by: Joe Elliott <number101010@gmail.com>

* lint + guardcode

Signed-off-by: Joe Elliott <number101010@gmail.com>

* change the way we're ignoring tests

Signed-off-by: Joe Elliott <number101010@gmail.com>

* clean up url parsing

Signed-off-by: Joe Elliott <number101010@gmail.com>

* fixed url parsing and add test

Signed-off-by: Joe Elliott <number101010@gmail.com>

* adjust sanity check

Signed-off-by: Joe Elliott <number101010@gmail.com>

---------

Signed-off-by: Joe Elliott <number101010@gmail.com>
mattdurham pushed a commit to mattdurham/tempo that referenced this pull request Jun 18, 2026
@electron0zero
Split metrics slo out from search slo (grafana#3659)
44f064c 
In grafana#3584 we decided to count metrics queries as part of search slo. 

split metrics slo out of search because metrics are still experimental and should not share the SLO with search.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants

@joe-elliott @mdisibio