-
Notifications
You must be signed in to change notification settings - Fork 839
Clarify documentation on series endpoint #2953
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
Clarify documentation on series endpoint #2953
Conversation
2c973d7 to
9777c61
Compare
pracucci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this doc fix! I've left few comments and then we're good to go 👍
CHANGELOG.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any documentation update should be an [ENHANCEMENT]. Changes are application-level behavioural changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to [ENHANCEMENT]
docs/configuration/v1-guarantees.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Requiring the `__name__` label on queries (except for queries to Ingesters). | |
| - Requiring the `__name__` label on queries when querying the chunks storage (queries to ingesters or clusters running the blocks storage are not affected). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, used the suggested change.
docs/configuration/v1-guarantees.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments:
- I would name the API
/api/v1/series - This also affects
/api/v1/labelsand/api/v1/label/{name}/values - Maybe it's more clear if we say that the API endpoints only query data from ingesters (not long-term storage)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have clarified the API path and added the mentioned endpoints. Also added that data is only retrieved from ingesters, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Signed-off-by: Jakob Kartschall <[email protected]>
Signed-off-by: Jakob Kartschall <[email protected]>
9777c61 to
6def277
Compare
pracucci
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (modulo a couple of nits)
@pstibrany Could you also review, please?
Signed-off-by: Jakob Kartschall <[email protected]>
3aba513 to
313a78d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need a changelog entry for documentation, but otherwise LGTM :) Thanks!
What this PR does:
Clarify documentation about the
/seriesendpoint and adds a note about queries without__name__label to ingesters.Which issue(s) this PR fixes:
N/A
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]