Skip to content

Conversation

joe-elliott
Copy link
Contributor

@joe-elliott joe-elliott commented Apr 9, 2020

What this PR does:

  • Adds the query frontend to the all in one binary
  • Adjusts the way the Querier paths are registered to prevent collisions between the querier and frontend
  • Extended e2e single binary tests to cover label names and values
  • Added a changelog entry. This seemed like a large enough change to warrant it, but would be glad to remove.

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pull-request-size pull-request-size bot added size/L and removed size/M labels Apr 9, 2020
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott marked this pull request as ready for review April 9, 2020 20:47
@joe-elliott
Copy link
Contributor Author

cc @jtlisi

Ready for review!

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

👋 I've pushed a fix for the label names/values querying in the blocks storage (the edge case triggered in integration tests was when the written series timestamp >= now). The api.go refactoring LGTM. I just have a concern about the "single binary will be mysteriously unresponsive unless worker is working".

@joe-elliott joe-elliott requested a review from pracucci April 10, 2020 12:51
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Thanks @joe-elliott for addressing my feedback! LGTM

Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM with a query. I wonder if we can expose the querier endpoints on a different path (with a prefix of /querier/ or something similar).

a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/user_stats", http.HandlerFunc(distributor.UserStatsHandler), true)
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/chunks", querier.ChunksHandler(queryable), true)

// these routes are either registered the default server OR to an internal mux. The internal mux is
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, does this mean that when using the single binary mode, do we not expose the querier endpoints at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is correct. I purposefully chose this route over path prefixing because I found it cleaner.

@gouthamve
Copy link
Contributor

I don't have a strong opinion here, but we should add a TODO in the comments before merging this :)

@gouthamve
Copy link
Contributor

Other than that LGTM!

Signed-off-by: Joe Elliott <[email protected]>
@pracucci pracucci merged commit cc8e02c into cortexproject:master Apr 14, 2020
@pracucci pracucci mentioned this pull request Apr 23, 2020
3 tasks
simonswine pushed a commit to grafana/e2e that referenced this pull request Jan 13, 2022
* Added query frontend to the single binary

Signed-off-by: Joe Elliott <[email protected]>

* First pass single binary

Signed-off-by: Joe Elliott <[email protected]>

* Pass parameter correctly

Signed-off-by: Joe Elliott <[email protected]>

* lint

Signed-off-by: Joe Elliott <[email protected]>

* Added label tests to single binary

Signed-off-by: Joe Elliott <[email protected]>

* Added a warning for bad single process config

Signed-off-by: Joe Elliott <[email protected]>

* Added auto config of worker

Signed-off-by: Joe Elliott <[email protected]>

* Always add auth middleware.

Signed-off-by: Joe Elliott <[email protected]>

* Fixed label tests and used address that worked in e2e tests

Signed-off-by: Joe Elliott <[email protected]>

* updated changelog

Signed-off-by: Joe Elliott <[email protected]>

* Run unbounded queries to fetch label names/values in the blocks storage

Signed-off-by: Marco Pracucci <[email protected]>

* Moved automatic worker config attempt above worker

Signed-off-by: Joe Elliott <[email protected]>

* Moved changelog entry and expanded

Signed-off-by: Joe Elliott <[email protected]>

* Change => Enhancement

Signed-off-by: Joe Elliott <[email protected]>

* Added todo

Signed-off-by: Joe Elliott <[email protected]>

Co-authored-by: Marco Pracucci <[email protected]>
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