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

[ENHANCEMENT] Add mimir-http-prefix option to introduce a prefix for Mimir URL when use legacy routes. #8069

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ying-jeanne
Copy link
Contributor

@ying-jeanne ying-jeanne commented May 6, 2024

[ENHANCEMENT] Add mimir-http-prefix option to introduce a prefix for Mimir URL when use legacy routes.

What this PR does

clean up some mimirtool bugs, adding mimir-http-prefix for mimir client when use legacy routes

Which issue(s) this PR fixes or relates to

Fixes #8018

Checklist

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

@ying-jeanne ying-jeanne marked this pull request as ready for review May 6, 2024 19:31
@ying-jeanne ying-jeanne requested a review from a team as a code owner May 6, 2024 19:31
var err error
if path, err = url.JoinPath(cfg.MimirHTTPPrefix, legacyAPIPath); err != nil {
return nil, err
}
}
Copy link
Contributor

@lamida lamida May 9, 2024

Choose a reason for hiding this comment

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

Nit. Should we validate that the user mustn't set MimirHTTPPrefix if UseLegacyRoutes is false? MimirHTTPPrefix is a new config, I think it might be better to strictly only allow this config when UseLegacyRoutes is set, instead of just ignoring the config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This might be tricky to check here, since MimirHTTPPrefix has a default value /prometheus, in client we will not able to distinguish the not set and default value.

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lamida lamida left a comment

Choose a reason for hiding this comment

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

In general LGTM. But I left two nit comments.

Co-authored-by: Jon Kartago Lamida <jon.lamida@grafana.com>
@ying-jeanne ying-jeanne requested a review from lamida May 13, 2024 15:28
Copy link
Contributor

@lamida lamida left a comment

Choose a reason for hiding this comment

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

LGTM

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.

mimirtool load rules not working with api.prometheus_http_prefix different than /prometheus
2 participants