-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Enables Series API in loki #1419
Conversation
/cc @davkal |
Cool! Let me know when this is merged. |
From a consumer point of view, I need a structure that ends up like this:
to suggest the right label sub tree at this caret entry point: This is something I can create from the result example above, but perhaps it's worth adding a second API endpoint that gives me straight up what I need :) |
We won't change this path, for compatibility with Prometheus but we can have another path for what you need. Any suggestions of a good path for this? |
Effectively I'm querying for labels, so it should be some sub path of the labels api? Edit: But I don't want to hold up this PR :P |
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, with one 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.
Great work @owen-d! This is looking good. Aside from my comments, I'd also like to see the API doc updated to reference the new endpoint.
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, but I'd still like to see docs/api.md
updated.
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.
Very good job @owen-d! Unfortunately I didn't have time to review it before it was merged, anyway LGTM. I just noticed that the series deduplication suffers hash collisions (which is a thing we're trying to move away from in Cortex and I guess is desiderable to not have in Loki too, but I agree it's not easy to trigger an xxhash collision).
|
||
func (q *Querier) awaitSeries(ctx context.Context, req *logproto.SeriesRequest) (*logproto.SeriesResponse, error) { | ||
|
||
// buffer the channels to the # of calls they're expecting su |
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.
What does expecting su
mean?
What
This PR implements a series api equivalent in loki, which enables resolving log streams from a list of matchers.
Examples
Problems/Prior art
Chunk involvement
We currently need to iterate through chunks for this feature because labels are stored inside chunks and not in indices.
How Prometheus does it
Prometheus does this in a very similar way, by querying for all matching series within a time range and plucking the labels.
Possible improvements
SeriesIDs as uniqueness
Starting in the v9 schema,
SeriesID
s are encoded in the index. There's even unexported functions lookupSeriesByMetricNameMatchers and lookupChunksBySeries to support it. Assuming that a sha256 of the labels is reasonable as a measure of uniqueness (seems so to me), we could make the upstream change and implement it this way. Then, we'd only need to pull one chunk per series instead of every chunk in the time range.I avoided parsing the
seriesID
out of the chunk's key. This may be feasible, if a bit hacky, but would allow us to fetch only one chunk per stream.Storing labels in indices
Labels could also be stored in the indices, removing the need to pull any chunks from storage.
Related
closes #113