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

Support Accept: application/json on common HTTP endpoints #2673

Merged
merged 9 commits into from
Jun 2, 2020

Conversation

joe-elliott
Copy link
Contributor

@joe-elliott joe-elliott commented Jun 1, 2020

What this PR does:

  • Adds support for the Accept: application/json header to the following endpoints:
  /distributor/all_user_stats
  /distributor/ha_tracker
  /ingester/ring
  /store-gateway/ring
  /compactor/ring
  /ruler/ring
  /services
  • Renders the /services endpoint in the standard Cortex html style

Which issue(s) this PR fixes:

Checklist

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

Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Copy link
Contributor Author

joe-elliott commented Jun 2, 2020

Parsing Accept correctly is non-trivial:
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept

More generally there is some discussion about adding support for content negotiation to the standard library here:
golang/go#19307

However, for our purposes I think a simple string.Contains is sufficient since there is no standard implementation, but I would be willing to research and use an existing implementation of Accept parsing if desired.

@joe-elliott joe-elliott marked this pull request as ready for review June 2, 2020 02:00
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott joe-elliott changed the title Accepts json Support Accept: application/json on common HTTP endpoints Jun 2, 2020
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

Our V1 Guarantees page explicitly says

  • Additional API endpoints for management of Cortex itself, such as the ring. These APIs are not part of the any compatibility guarantees.

If we are clear that these endpoints may change and break at any point, we can keep them.

Proper handling of accepts (with priorities) would be nice, but since browsers are typically not setting application/json, this should be fine for now.

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!

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.

LGTM, thanks!

@pracucci pracucci merged commit 80b1261 into cortexproject:master Jun 2, 2020
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