Skip to content

API Design Proposal #2327

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Apr 8, 2020
Merged

API Design Proposal #2327

merged 8 commits into from
Apr 8, 2020

Conversation

jtlisi
Copy link
Contributor

@jtlisi jtlisi commented Mar 24, 2020

What this PR does:
This PR adds an initial draft of a proposal to restructure and version the Cortex API.

pracucci
pracucci approved these changes Mar 25, 2020
View reviewed changes
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! I left few comments here and there, but I 💯 agree with the overall proposal.

pstibrany
pstibrany approved these changes Mar 25, 2020
View reviewed changes
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.

Good idea, LGTM

khaines
khaines approved these changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@khaines khaines left a comment

Choose a reason for hiding this comment

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

LGTM. I'm very happy to see this written down @jtlisi

gouthamve
gouthamve reviewed Mar 27, 2020
View reviewed changes
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.

Looks good right now! Waiting for you to resolve the existing conversations before I approve!

docs/proposals/api_design.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link to the github issue as well?

docs/proposals/api_design.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

/read is a Prometheus API endpoint.

Copy link
Contributor Author

@jtlisi jtlisi Mar 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@gouthamve I noticed. It's a bit strange since we currently host the read endpoint under /api/prom/read. It should be /api/prom/api/v1/read. In this case it make sense to move both of these endpoints under the Prometheus API:

/prometheus/api/v1/read & /prometheus/api/v1/write WDYT?

/write is not technically part of the Prometheus API. However, the spec for writes is defined by the project so it might make sense to embed it into the Prometheus API.

@jtlisi jtlisi mentioned this pull request Mar 31, 2020
Merged
3 tasks
@pracucci
Copy link
Contributor

pracucci commented Apr 6, 2020

Can we consider the proposal accepted and merge it?

@jtlisi
Copy link
Contributor Author

jtlisi commented Apr 7, 2020

I'm fine with merging it now.

@pracucci pracucci force-pushed the 20200324_api_design_proposal branch from e107433 to 2d4b6f4 Compare April 8, 2020 07:35
@pracucci
Copy link
Contributor

pracucci commented Apr 8, 2020

@jtlisi I rebased master and fixed the doc page header to have a nice formatting in the website. I will merge once CI passes.

@pracucci
Copy link
Contributor

pracucci commented Apr 8, 2020

The failed lint is unrelated and will be fixed in a separate PR.

@pracucci pracucci merged commit ab97b84 into cortexproject:master Apr 8, 2020
@pracucci pracucci deleted the 20200324_api_design_proposal branch April 8, 2020 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
4 more reviewers

@pracucci pracucci pracucci approved these changes

@gouthamve gouthamve gouthamve left review comments

@pstibrany pstibrany pstibrany approved these changes

@khaines khaines khaines approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jtlisi @pracucci @pstibrany @khaines @gouthamve