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

[Change Proposal] Allow for nested category structure #451

Closed
kpollich opened this issue Nov 22, 2022 · 15 comments
Closed

[Change Proposal] Allow for nested category structure #451

kpollich opened this issue Nov 22, 2022 · 15 comments
Assignees
Labels
discuss Issue needs discussion Team:Ecosystem Label for the Packages Ecosystem team

Comments

@kpollich
Copy link
Member

We're actively rethinking how we categorize all Elastic Agent integrations. Part of this reimagining includes devising a category + subcategory organization structure for certain integrations (e.g. security) with more nuanced structure than a single category can provide.

We'll need to support integrations defining a top-level category as well as an optional subcategory. This means updating the rules around categories for each package's spec, as well as the logic that drives the https://epr.elastic.co/categories API.

We'll need to add support for the new structure in Kibana, but that will be a separate issue referenced later.

@kpollich kpollich added the discuss Issue needs discussion label Nov 22, 2022
@jlind23 jlind23 added the Team:Ecosystem Label for the Packages Ecosystem team label Nov 22, 2022
@kpollich
Copy link
Member Author

cc @amitkanfer @dborodyansky

@jsoriano
Copy link
Member

The categories endpoint has always been a bit tricky and a source of bugs and corner cases, I hope we can do this without adding a lot of complexity.

Maybe an option is to keep categories simple in packages, with the current categories field, but adding the level information to the API responses, something like this:

  ...
  {
    "id": "security",
    "title": "Security",
    "count": 95
  },
  {
    "id": "threat_intel",
    "title": "Threat Intelligence",
    "subcategory_of": "security",
    "count": 9
  },
  ...

Then the UIs can build the tree of categories from that.

We'd need to think where to configure these categories. Some of their information is already hard-coded, I guess we can also hard-code the known sub-categories. We could also consider bringing this information to the package-spec and consuming it from there.

@jlind23
Copy link
Collaborator

jlind23 commented Nov 23, 2022

@jsoriano @kpollich I believe that adding a new field to the package spec is definitely the best solution, adding it to the package spec V2 will enforce encourage people to migrate. What do you think?

@jsoriano
Copy link
Member

I believe that adding a new field to the package spec is definitely the best solution

@jlind23 adding it as a new field has some problems in my opinion:

  • It adds more complexity. New field in the manifest and something else we have to take into account in the package registry when handling searches. Kibana/Fleet also needs to take care of both fields.
  • Reorganizing categories into subcategories would be a breaking change. Or we need to duplicate categories and subcategories to avoid breaking changes, potentially introducing complexity to deduplicate them on search results.

adding it to the package spec V2 will enforce encourage people to migrate

We can also encourage the use of v2 while keeping a single field, if we only add the new categories to latest version.

The one point where I see that adding a new field can have an actual advantage is on support of old versions of kibana/fleet. We can add new categories and they can just ignore them, but adding subcategories into the current categories field will make the list of categories longer. This may be not such an issue though, and we could try to mitigate it from the registry by not exposing subcategories on requests for old versions of kibana.

@amitkanfer
Copy link

@jsoriano what's your recommendation?

@kpollich
Copy link
Member Author

Ideally, packages could continue using the categories the way they do now, just defining one or more categories in their manifest. We need to represent the tree structure w/ parent/child nodes elsewhere, and keep packages out of that concern.

We'd need to think where to configure these categories. Some of their information is already hard-coded, I guess we can also hard-code the known sub-categories. We could also consider bringing this information to the package-spec and consuming it from there.

Could we create some top-level categories object exported by the package spec that defines the tree structure of categories? Could be a YML file that informs the package registry on how to build the categories API endpoint JSON.

@jsoriano
Copy link
Member

@jsoriano what's your recommendation?

I would use the current field we have for categories in packages. As Kyle mentions, this will help to keep packages out of this concern. And will allow us to change the level of a category without producing breaking changes, as would happen for example if we move thread_intel from being a first-level category to be a subcategory of security.

The registry API will need to be extended to include this new relationship between categories as mentioned in #451 (comment).

An alternative for the API change could be to reproduce the tree structure. This would prevent old versions of kibana from displaying too many categories when we start adding subcategories, as they would only use the first level. But it could make some categories to disappear if we move them to subcategories. A response implemented this way would look like the following:

  ...
  {
    "id": "security",
    "title": "Security",
    "count": 95
    "subcategories": [
      {
        "id": "threat_intel",
        "title": "Threat Intelligence",
        "count": 9
      },
      ...
    ],
    ...
  ...

Could we create some top-level categories object exported by the package spec that defines the tree structure of categories? Could be a YML file that informs the package registry on how to build the categories API endpoint JSON.

Yes, we can explore something like this, having it in a yaml file can help product or design to make changes there.

tree structure

I would keep it simple and allow only one level of subcategories, or do we expect the necessity of having more levels?

@kpollich
Copy link
Member Author

I would keep it simple and allow only one level of subcategories, or do we expect the necessity of having more levels?

Good call out - let's plan to only support one level of nesting for now. So a single level of subcategories per category.

@jsoriano
Copy link
Member

I have started with the definition of the subcategories in elastic/package-registry#914, I have used the list of subcategories prepared by @dborodyansky by now.

Please take a look to the comments in the definitions https://github.com/elastic/package-registry/pull/914/files#diff-51ada513c4a7741a37782b2888f80297dd8c8a78377579a7409ff396b33edb4f

@jsoriano
Copy link
Member

jsoriano commented Dec 5, 2022

Changes in the registry for nested categories merged.

@kpollich should we also update the search API, so when searching for a category, it also returns the packages belonging to its subcategories?

That would be that for example, for /search?category=infrastructure, package registry returns all packages belonging to the infrastructure category or its subcategories, including analytics_engine, load_balancer, web....
Currently this request will only return packages belonging explicitly to the infrastructure category.

@kpollich
Copy link
Member Author

kpollich commented Dec 5, 2022

That would be that for example, for /search?category=infrastructure, package registry returns all packages belonging to the infrastructure category or its subcategories, including analytics_engine, load_balancer, web....

This seems proper, but we don't have a dependency in the integrations UI for this functionality. We load all the categories up front from /categories and store them in memory - we don't make multiple requests to the /search API from EPR as far as I can tell. We don't have any requests to /search that include the ?category query param that I can find with a quick grep.

Maybe we could make this a low priority tech debt task, or if you think it's something that'd be very straightforward to implement quickly we can take care of it sooner. What do you think, @jsoriano?

@jsoriano
Copy link
Member

jsoriano commented Dec 5, 2022

I think this would be a quick change, I have opened a draft PR here: elastic/package-registry#921.

If kibana is not currently filtering by category I think we can make the change as this won't affect anything now, but can make sense to have in the future if we start filtering by category.

@jlind23
Copy link
Collaborator

jlind23 commented Dec 7, 2022

@jsoriano @kpollich what is missing here to consider this issue as done? A release of the registry api might be needed by I believe that this is the last step. @kpollich we might to create a Kibana issue to follow up on the UI design

@jsoriano
Copy link
Member

jsoriano commented Dec 7, 2022

A release of the registry api might be needed by I believe that this is the last step.

Yes, package registry 1.16.0 was released yesterday including this change, and epr.elastic.co will hopefuly be updated today.

Change in kibana is pending, we can close this issue once there is an issue or PR open for it.

@jlind23
Copy link
Collaborator

jlind23 commented Dec 7, 2022

Closing this as done then and we will follow up with this kibana issue: elastic/kibana#147125

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs discussion Team:Ecosystem Label for the Packages Ecosystem team
Projects
None yet
Development

No branches or pull requests

4 participants