-
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
feat: Store structured metadata with patterns #12936
Conversation
} | ||
|
||
func newChunk(ts model.Time) Chunk { | ||
func newChunk(ts model.Time, structuredMetadata push.LabelsAdapter) Chunk { |
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'm not confident I am using the right types to pass the labels through from the request - any recommendations here?
Value: v, | ||
}) | ||
} | ||
_, _, matches := labelFilters.Process(0, emptyLine, chunkMetadata...) |
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.
This feels a little hacky since I'm passing empty TS and Line, but it enables re-use of the existing LogQL logic to implement the matching so I think its the best option.
if err != nil { | ||
return nil, httpgrpc.Errorf(http.StatusBadRequest, err.Error()) | ||
} | ||
var queryErr error | ||
expr.Walk(func(treeExpr syntax.Expr) { |
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 put this here to guard against anyone passing in more complex queries to this API. Only allowing a subset of LogQL is a bit of an odd use case so I'd appreciate any input on whether there's a better way to do this.
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.
The way you did it is fine, but it duplicates data this is why I think we want to improve storage to co-locate all this data together.
The strategy I'm currently working on for storing series/metrics might help here, where we can store the label on the metric series, since that's really what we want from this API (the metric for the counts that is) |
Pausing this until #13020 is merged, then we'll rethink whether this approach will work. |
What this PR does / why we need it:
| detected_level="debug"
can be applied to patterns returned by the API.I'm definitely not confident this is the right way to implement this feature, hence leaving this PR in draft mode until I get some early feedback.
It also needs some more tests so I'll follow up with some more commits over the next few days before making undrafting this PR.