-
Notifications
You must be signed in to change notification settings - Fork 472
[Enhancement] Update categories for packages #14571
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
base: main
Are you sure you want to change the base?
Conversation
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 just reviewed the changes to "windows" package since that is the only one owned by elastic-agent-data-plane.
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 addition of the network_security
category in packages/beaconing
should work; cc @sodhikirti07
🚀 Benchmarks reportTo see the full report comment with |
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.
Without elastic/package-spec providing any description of the category meanings, there is some subjectivity to the labeling. I have brought my own assumptions to this review.
Overall looks good, it probably would be good for PMs that know these products to also have a look.
Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane) |
Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices) |
Pinging @elastic/sec-applied-ml (Team:Security-Applied ML) |
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform) |
Added @daniela-elastic to help review the o11y package changes. |
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.
What about universal_profiling_collector and universal_profiling_symbolizer?
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.
you suggest to update categories to include observability
?
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 have updated it
packages/activemq/manifest.yml
Outdated
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.
Could we say that ActiveMQ is part of the infrastructure stack and what is the definition of infra stack? Asking because we have an integration Kafka which one could argue is an alternative to ActiveMQ (minus the streaming part) however today Kafka integration doesn't have infra category. Perhaps we should look into conformity between the various similar family of services to make sure they are categorized in the same way.
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.
Thanks for taking the initiative to update the categories! It's been long overdue. Provided some suggestions, hopefully those make sense and can be easily addressed
@daniela-elastic I have updated code according to your comments |
💔 Build Failed
Failed CI StepsHistory
|
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 changes to universal_profiling_*
packages look good to me. For the rest of the packages the respective people have to speak up.
|
We are using categories of packages in telemetry and it seems that categories of packages are not reliable as I found a few of security packages not marked as such, for example
aws
etc.I took advantage of LLM to update the list of categories according to the readme in each integration and using this list of available categories
I have verified it as much as I could but as it touches a lot of integrations it would be great to have more eyes on it.
Every new category has justification
p.s. I am new to the integrations development so it might be I’m missing some of the usages of categories and current changes might break something. If so - I am happy to update the PR
FYI @vglagoleva @andrewkroh