-
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
[Cloud Security] Refactor D4C metering function #183814
Conversation
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
@elasticmachine merge upstream |
x-pack/plugins/security_solution_serverless/server/cloud_security/constants.ts
Outdated
Show resolved
Hide resolved
df40095
to
41ebf91
Compare
@elasticmachine merge upstream |
@elasticmachine merge upstream |
5e24f64
to
0e39cfc
Compare
cc7dcec
to
4cfd3f0
Compare
{ | ||
index: CLOUD_DEFEND_HEARTBEAT_INDEX, | ||
sort: 'event.ingested', | ||
query: { |
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.
We probably need an explicit size
param here otherwise it will only pull 10 docs per run and miss the other heartbeats.
@@ -25,20 +26,31 @@ export const cloudSecurityMetringCallback = async ({ | |||
const tier: Tier = getCloudProductTier(config, logger); | |||
|
|||
try { | |||
const cloudSecuritySolutions: CloudSecuritySolutions[] = [CSPM, KSPM, CNVM, CLOUD_DEFEND]; | |||
const cloudSecuritySolutions: CloudSecuritySolutions[] = [CLOUD_DEFEND]; | |||
|
|||
const promiseResults = await Promise.allSettled( |
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.
It might be good to additionally return a shouldRunAgain and latestTimestamp along with the records so that if there are more heartbeat documents, the task will run again immediately to pick up the rest of the heartbeats.
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.
Your approach makes sense to me so I went ahead and approved. Something to keep in mind with this approach is that if there is a very large amount of heartbeats the task might potentially run into the 1 minute timeout.
e72fc9b
to
52e81f8
Compare
@elasticmachine merge upstream |
}; | ||
} | ||
|
||
const buildMeteringRecord = ( |
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.
const buildMeteringRecord = ( | |
const buildUsageRecord = ( |
); | ||
}; | ||
|
||
export const getCloudDefendUsageRecord = async ({ |
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.
export const getCloudDefendUsageRecord = async ({ | |
export const getCloudDefendUsageRecords = async ({ |
if (usageRecords.hits.hits.length < BATCH_SIZE) { | ||
fetchMore = false; | ||
} else { | ||
searchAfter = usageRecords.hits.hits[usageRecords.hits.hits.length - 1].sort; |
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.
does this return the latest timestamp from the previous batch?
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.
yep:)
timestamp.setMilliseconds(0); | ||
|
||
const usageRecord = { | ||
id: `container-defend-${agentId}-${timestamp.toISOString()}`, |
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'm not sure I understood correctly - we do create a usage, record record per heartbeat, but we trust the id uniqueness to squash them into a single record every half an hour?
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 agent sends a heartbeat every 30 minutes. We want to count records with an hourly granularity, so every 2 records will have the same record ID. Later, in the glue function, they will be merged into one.
54422b0
to
444fd4d
Compare
@elasticmachine merge upstream |
a83d403
to
69e93eb
Compare
@@ -153,7 +153,7 @@ export class SecurityUsageReportingTask { | |||
return { state: taskInstance.state, runAt: new Date() }; | |||
} | |||
|
|||
this.logger.debug(`received usage records: ${JSON.stringify(usageRecords)}`); | |||
this.logger.warn(`received usage records: ${JSON.stringify(usageRecords)}`); // TODO: change to info before merge |
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.
@CohenIdo this is still warn
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Canvas Sharable Runtime
History
To update your PR or re-run it, just comment with: |
Summary