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

Web route prefix is not applied on debug endpoints. #2486

Closed
yeya24 opened this issue Apr 21, 2020 · 22 comments
Closed

Web route prefix is not applied on debug endpoints. #2486

yeya24 opened this issue Apr 21, 2020 · 22 comments

Comments

@yeya24
Copy link
Contributor

yeya24 commented Apr 21, 2020

In Prometheus, if the web external path is set, then all Prometheus routes will be updated. There is a good example here https://www.robustperception.io/using-external-urls-and-proxies-with-prometheus

You should be aware that with this external URL, the /prometheus/ path prefix will be required for all HTTP access to Prometheus. 

The /metrics will be on http://localhost:9090/prometheus/metrics for example.

However, in thanos, if web route prefix is set to /thanos, only /api/v1 endpoints will be updated to /thanos/api/v1. But other endpoints like /metrics, still serve on /metrics, we should add web route prefix to these endpoints as well.

AC:

  • Endpoints like /metrics or /-/ready can work with web route prefix just like Prometheus.
@fengzixu
Copy link

/assign @fengzixu

@bwplotka bwplotka changed the title web route prefix is not correctly handled for all routes v0.12.0 regression: Web route prefix is not correctly handled for all routes. Apr 21, 2020
@bwplotka
Copy link
Member

Fixed on relase branch, v0.12.1 should have it

@yeya24
Copy link
Contributor Author

yeya24 commented Apr 21, 2020

This is not done. #2489 only fixes the problem of duplicate registering the web route prefix. This issue more focused on handling some other endpoints with web route prefix. Such as /metrics and /-/ready, to be consistent with the endpoints in Prometheus.

@yeya24 yeya24 reopened this Apr 21, 2020
@bwplotka
Copy link
Member

Sorry, you are right, I confused things. Thanks @yeya24 !

@yeya24
Copy link
Contributor Author

yeya24 commented Apr 21, 2020

No worries. @bwplotka Feel free to edit the issue description, if it is not clear to you. Maybe we should update the title?

@bwplotka bwplotka changed the title v0.12.0 regression: Web route prefix is not correctly handled for all routes. Web route prefix is not applied on debug endpoints. Apr 21, 2020
@ranjithkumar007
Copy link
Contributor

@fengzixu are you working on this issue?

@stale
Copy link

stale bot commented May 23, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label May 23, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented May 25, 2020

Valid #2524

@stale stale bot removed the stale label May 25, 2020
@stale
Copy link

stale bot commented Jun 24, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 24, 2020
@ranjithkumar007
Copy link
Contributor

ranjithkumar007 commented Jun 25, 2020

Hi @stalebot, I will update this.

@stale stale bot removed the stale label Jun 25, 2020
@stale
Copy link

stale bot commented Jul 25, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jul 25, 2020
@ranjithkumar007
Copy link
Contributor

calm down @stale-bot

@stale stale bot removed the stale label Jul 25, 2020
@stale
Copy link

stale bot commented Aug 1, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Aug 1, 2020
@yeya24 yeya24 reopened this Aug 1, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented Aug 1, 2020

This pr should be review #2524, just need another 👁️ to take a look.

@stale
Copy link

stale bot commented Aug 31, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Aug 31, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented Aug 31, 2020

Need another review.

@stale stale bot removed the stale label Aug 31, 2020
@stale
Copy link

stale bot commented Oct 30, 2020

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 30, 2020
@kakkoyun kakkoyun removed the stale label Oct 31, 2020
@stale
Copy link

stale bot commented Dec 31, 2020

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Dec 31, 2020
@yeya24
Copy link
Contributor Author

yeya24 commented Dec 31, 2020

Still valid.

@stale stale bot removed the stale label Dec 31, 2020
@stale
Copy link

stale bot commented Mar 5, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Mar 5, 2021
@onprem onprem removed the stale label Mar 7, 2021
@stale
Copy link

stale bot commented Jun 2, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 2, 2021
@stale
Copy link

stale bot commented Jun 16, 2021

Closing for now as promised, let us know if you need this to be reopened! 🤗

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants