-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] add back monitoring for agentless policies #191635
Conversation
Pinging @elastic/fleet (Team:Fleet) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
@@ -848,7 +848,7 @@ describe('When on the package policy create page', () => { | |||
|
|||
expect(sendCreateAgentPolicy).toHaveBeenCalledWith( | |||
expect.objectContaining({ | |||
monitoring_enabled: [], | |||
monitoring_enabled: ['logs', 'metrics', 'traces'], |
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 @juliaElastic!
I don't think we had traces included originally
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.
Traces were recently added in this pr: #189908
And the logic takes all values:
monitoring_enabled: Object.values(dataTypes), |
I can remove
traces
if we don't want it in agentless.
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.
@amirbenun I remember you looked into it.
Do you think we need traces enabled?
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.
Since I am not familiar with that I am not sure how it's going to affect the user.
I think it's best to have only logs
and metrics
which were both tested and we might add traces
in the future.
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.
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.
Traces may help with [Cloud Security] [Agentless] Include X-Request-ID header in all Agentless API requests and logs
logs
is mainly what we need for the correlation, looking at what traces offers, it can help users detect issues with the communication with agentless, It could be useful during the agentless beta, however, I am not sure if traces would expose internal services to the users such as the agentless endpoints and requests/responses, which is something we had not initially planned.
So I tend to agree with @amirbenun to enable only logs
and metrics
for now and evaluate if we can enable traces in the future
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.
Updated with only logs and metrics as requested.
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.
LGTM
/ci |
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.
Assuming the trace discussion is resolved, I approve.
@@ -848,7 +848,7 @@ describe('When on the package policy create page', () => { | |||
|
|||
expect(sendCreateAgentPolicy).toHaveBeenCalledWith( | |||
expect.objectContaining({ | |||
monitoring_enabled: [], | |||
monitoring_enabled: ['logs', 'metrics', 'traces'], |
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.
/ci |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Added back agent monitoring on agentless policies as requested. It was incorrectly removed in #189612