-
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] using point in time for agent status query to avoid discrepancy #135816
Conversation
Pinging @elastic/fleet (Team:Fleet) |
kuery: joinKuerys( | ||
...[ | ||
kuery, | ||
filterKuery, | ||
`${AGENTS_PREFIX}.attributes.active:true`, |
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.
adding active filter here is incorrect, as getAgentsByKuery
adds it based on showInactive
:
kibana/x-pack/plugins/fleet/server/services/agents/crud.ts
Lines 125 to 127 in bce1836
if (showInactive === false) { | |
filters.push(ACTIVE_AGENT_CONDITION); | |
} |
await closePointInTime(esClient, pitId); | ||
} | ||
|
||
const result = { | ||
total: allActive.total, |
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 noticed that total
doesn't include inactive
, is it intentional? It can be a little confusing, example:
{
"results": {
"total": 981,
"inactive": 20,
"online": 981,
"error": 0,
"offline": 0,
"updating": 0,
"other": 20,
"events": 0
}
}
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.
Inactive agents are the ones that have been unenrolled, so they're generally excluded from the status as they're not relevant most of the time. We should probably have two fields, one for the real total and one for the total active, but I think we should only do this in a non-breaking way. Maybe total
(all active, deprecated) and all
(all active and inactive) and active
(all active)
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 agree, this would be cleaner.
Do you happen to know if there is any usage of other
status? It might be good to deprecate that as well, as it seems confusing. It is not mentioned in the UI/docs either.
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 agree it is confusing. Total seems to be total active, which you don't assume from the name total, my vote would be to fix this.
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.
Raised an issue to deprecate total status: #135980
@@ -54,6 +54,7 @@ export const TableRowActions: React.FunctionComponent<{ | |||
onClick={(event) => { | |||
onAddRemoveTagsClick((event.target as Element).closest('button')!); | |||
}} | |||
disabled={!agent.active} |
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.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
@juliaElastic should we backport this to 8.3? |
Summary
Fix #134798
getAgentsByKuery
andgetAgentsByKueryPit
functionsshowInactive: false
parameter because there was an unnecessaryactive:true
filter added.Tested by:
GET kbn:/api/fleet/agent_status
online + offline > total
- did this by logging out when this happensChecklist