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

feat: Store structured metadata with patterns #12936

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

benclive
Copy link
Contributor

What this PR does / why we need it:

  • Adds structured metadata from the incoming log stream to the Pattern chunks.
  • Enables the the use of label filters in the Pattern API Query string so filters like | detected_level="debug" can be applied to patterns returned by the API.
  • No other LogQL queries are supported.

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.

}

func newChunk(ts model.Time) Chunk {
func newChunk(ts model.Time, structuredMetadata push.LabelsAdapter) Chunk {
Copy link
Contributor Author

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...)
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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.

Copy link
Contributor

@cyriltovena cyriltovena left a 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.

@trevorwhitney
Copy link
Collaborator

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)

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.

None yet

3 participants