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

Allow HTTP pushes directly to ingesters. #1491

Merged
merged 6 commits into from
Feb 26, 2020
Merged

Conversation

tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented Jul 6, 2019

This is mostly for benchmark comparisons.

Signed-off-by: Tom Wilkie tom.wilkie@gmail.com

@bboreham
Copy link
Contributor

bboreham commented Jul 8, 2019

We should drop that "billing" stuff. Unfortunately I probably need to put something else in its place or I'll spook the natives.

@tomwilkie
Copy link
Contributor Author

Agreed re: billing. We use the per-user metrics for billing now.

@tomwilkie tomwilkie force-pushed the allow-push-to-ingester branch from 7d75297 to 98db9d1 Compare February 22, 2020 14:23
@tomwilkie tomwilkie marked this pull request as ready for review February 22, 2020 14:23
@tomwilkie
Copy link
Contributor Author

@bboreham I've remove the billing code, WDYT?

@bboreham
Copy link
Contributor

I’m still at “ need to put something else in its place”.
Also you should mention this change in the title and in changelog.

@tomwilkie tomwilkie force-pushed the allow-push-to-ingester branch 2 times, most recently from e010af7 to 2556d88 Compare February 22, 2020 18:58
@tomwilkie
Copy link
Contributor Author

I’m still at “ need to put something else in its place”.

Do you want us to wait? When do you think you'll have a chance to do this?

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.

Changes LGTM. I've just a concern about the API path (see the comment), everything else looks good.

@@ -341,6 +342,7 @@ func (t *Cortex) initIngester(cfg *Config) (err error) {
t.server.HTTP.Path("/ready").Handler(http.HandlerFunc(t.ingester.ReadinessHandler))
t.server.HTTP.Path("/flush").Handler(http.HandlerFunc(t.ingester.FlushHandler))
t.server.HTTP.Path("/shutdown").Handler(http.HandlerFunc(t.ingester.ShutdownHandler))
t.server.HTTP.Handle("/push", t.httpAuthMiddleware.Wrap(push.Handler(cfg.Distributor, t.ingester.Push)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

  1. I'm a bit concerned about the inconsistency in the path between distributors (/api/prom/push) and ingesters (/push). Have you thought about it?
  2. Could you document it to docs/apis.md, please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit concerned about the inconsistency in the path between distributors (/api/prom/push) and ingesters (/push). Have you thought about it?

Yeah, we can have them at the same path as the paths will collide in the single binary case.

I put it at /push to indicate its a private API; the /api/prom is for public endpoints.

@bboreham
Copy link
Contributor

I’m still at “ need to put something else in its place”.
Do you want us to wait? When do you think you'll have a chance to do this?

I have written the program. I don't think it's essential that you wait.

tomwilkie and others added 6 commits February 26, 2020 12:30
…sters.

Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom.wilkie@gmail.com>
Signed-off-by: Tom Wilkie <tom@grafana.com>
Signed-off-by: Tom Wilkie <tom@grafana.com>
Signed-off-by: Tom Wilkie <tom@grafana.com>
@tomwilkie tomwilkie force-pushed the allow-push-to-ingester branch from c7a3956 to 1b1c147 Compare February 26, 2020 12:34
@tomwilkie tomwilkie merged commit fc47e09 into master Feb 26, 2020
@tomwilkie tomwilkie deleted the allow-push-to-ingester branch February 26, 2020 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants