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

Implement content packages discovery #1235

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Oct 8, 2024

Fixes #1229.

Add a new discovery parameter to /search and /categories endpoints, to filter packages based on their discovery fields. It supports a list of fields that has to be passed in the form fields:<comma-separated list of fields>. To pass the filter, packages must include discovery fields in their manifest, and all of them must be included in the list included in the parameter.

For example the following request will return prerelease packages of content type, whose discovery subfields are a subset of ['http.method', 'apache.status.total_bytes'].

GET /search?prerelease=true&type=content&discovery=fields:http.method,apache.status.total_bytes

@jsoriano jsoriano self-assigned this Oct 8, 2024
@jsoriano
Copy link
Member Author

jsoriano commented Oct 8, 2024

@mrodm do you know how the search-index-all-full.json can be regenerated?

I would like to add content packages there.

@jsoriano jsoriano marked this pull request as ready for review October 8, 2024 11:52
@jsoriano jsoriano requested a review from a team October 8, 2024 11:52
@@ -219,7 +227,7 @@ func getCategoriesOutput(ctx context.Context, categories map[string]*packages.Ca
}
sort.Strings(keys)

var outputCategories []*packages.Category
outputCategories := []*packages.Category{}
Copy link
Member Author

Choose a reason for hiding this comment

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

Unrelated change. It changes the response of the /categories endpoint to generate [] instead of null when no packages satisfy the conditions.

@mrodm
Copy link
Contributor

mrodm commented Oct 8, 2024

@mrodm do you know how the search-index-all-full.json can be regenerated?

I would like to add content packages there.

The last time I had to add new packages (and update this file), I did it manually.
I don't know if there is any automated way to achieve this, sorry :(

CHANGELOG.md Outdated Show resolved Hide resolved
}

func NewDiscoveryFilter(filter string) (*DiscoveryFilter, error) {
filterType, args, found := strings.Cut(filter, ":")
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, it is supported just once type of discovery field so this looks good for a query like this:
GET /search?discovery=fields:process.pid,user.id

If it is needed to add a second type of discovery field, in order to get all the values of these query parameters, would it be needed to repeat the query parameter?

GET /search?discovery=fields:process.pid,user.id&discovery=searches:foo,bar

Copy link
Member Author

@jsoriano jsoriano Oct 8, 2024

Choose a reason for hiding this comment

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

Currently we only have a type of discovery method, so I didn't add support for more.

If we had, a request using more than one would require setting the parameter multiple times, yes.

I can add support for this, though it wouldn't be really useful at the moment. What do you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add support for several discovery methods now, just wanted to check one option about how to define another field in the query. Adding multiple times discovery looks good I think. 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think I should add support for discovery appearing multiple times?

So these two queries would be equivalent:

GET /search?discovery=fields:process.pid&discovery=fields:user.id
GET /search?discovery=fields:process.pid,user.id

This would also serve as base to add other methods. Though I don't think this would be very useful by now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, it is not required to do it now. I guess Kibana could put all the fields required in one query parameter, WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there would be no reason to separate in multiple parameters.

packages/packages.go Outdated Show resolved Hide resolved
packages/packages.go Show resolved Hide resolved
jsoriano and others added 2 commits October 8, 2024 16:17
Co-authored-by: Mario Rodriguez Molins <marrodmo@gmail.com>
@elasticmachine
Copy link

💚 Build Succeeded

History

cc @jsoriano

@jsoriano jsoriano merged commit 8899500 into elastic:main Oct 8, 2024
5 checks passed
@jsoriano jsoriano deleted the discovery-content-packages branch October 8, 2024 21:50
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.

Add support to index and search packages based on discovery features
3 participants