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

Enable metric.context filter in Aim metrics search #685

Merged
merged 20 commits into from
Dec 18, 2023

Conversation

suprjinx
Copy link
Contributor

@suprjinx suprjinx commented Dec 5, 2023

Ths PR adapts the query parser used by the Aim api to accept metric.context filters
Fixes #361

@suprjinx suprjinx changed the title WIP querying context.json with aim search Enable metric.context filter with aim metrics search Dec 7, 2023
@suprjinx suprjinx marked this pull request as ready for review December 7, 2023 16:01
@suprjinx suprjinx changed the title Enable metric.context filter with aim metrics search Enable metric.context filter in Aim metrics search Dec 7, 2023
}

func (eq JsonEq) Build(builder clause.Builder) {
eq.Left.Build(builder)
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 had to "override" these methods from the Gorm built-in so we could call Build on the left-hand portion, instead of writing quoted

Copy link
Collaborator

@fabiovincenzi fabiovincenzi left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-12-12 at 11 22 23 Tried a bit from the UI, and it seems to retrieve metrics that don't have the context key/value specified.

@suprjinx
Copy link
Contributor Author

Screenshot 2023-12-12 at 11 22 23 Tried a bit from the UI, and it seems to retrieve metrics that don't have the context key/value specified.

@fabiovincenzi I am not able to reproduce -- the filtering works correctly for me. Screenshots:
image
image

{
name: "SearchMetricContext",
request: request.SearchMetricsRequest{
Query: `metric.name == "TestMetric1" and metric.context.testkey == "testvalue"`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe using multiple metric.name in the query to see if different metrics are filtered by context, something like:
Query: ((metric.name == "TestMetric1") or (metric.name == "TestMetric2")) and metric.context.testkey == "testvalue"

{
name: "NegativeSearchMetricContext",
request: request.SearchMetricsRequest{
Query: `metric.name == "TestMetric1" and metric.context.testkey != "testvalue"`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

@suprjinx suprjinx merged commit 4f206e3 into G-Research:main Dec 18, 2023
17 checks passed
@suprjinx suprjinx deleted the json-context-query-aim branch December 18, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric contexts: adapt the Aim SearchMetrics endpoint to filter the query down based on the context
2 participants