-
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
[Cases] Adding file telemetry #152968
[Cases] Adding file telemetry #152968
Conversation
/** | ||
* This should only be used within telemetry | ||
*/ | ||
export const OWNERS = [OBSERVABILITY_OWNER, SECURITY_SOLUTION_OWNER, GENERAL_CASES_OWNER] as const; |
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 have these values in common now.
@@ -42,7 +43,7 @@ export const createCasesTelemetry = async ({ | |||
}: CreateCasesTelemetryArgs) => { | |||
const getInternalSavedObjectClient = async (): Promise<ISavedObjectsRepository> => { | |||
const [coreStart] = await core.getStartServices(); | |||
return coreStart.savedObjects.createInternalRepository(SAVED_OBJECT_TYPES); | |||
return coreStart.savedObjects.createInternalRepository([...SAVED_OBJECT_TYPES, FILE_SO_TYPE]); |
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.
Just calling this out, to retrieve the average file size we have to do an average aggregation over the file service's saved objects that contain a metadata field caseId
. Hopefully it's ok that we're accessing the file service's saved object internals.
The advantage of doing it this way is that we don't need to duplicate the file size within case comments.
createdAt: savedObjects?.[0].saved_objects?.[0].attributes?.created_at ?? null, | ||
updatedAt: savedObjects?.[1].saved_objects?.[0].attributes?.updated_at ?? null, | ||
closedAt: savedObjects?.[2].saved_objects?.[0].attributes?.closed_at ?? null, | ||
createdAt: savedObjects?.[0]?.saved_objects?.[0]?.attributes?.created_at ?? null, |
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.
When the telemetry runs initially and there are no cases this was throwing an error
}; | ||
}; | ||
|
||
export const getCasesTelemetryData = async ({ | ||
savedObjectsClient, | ||
logger, | ||
}: CollectTelemetryDataParams): Promise<CasesTelemetry['cases']> => { | ||
const byOwnerAggregationQuery = OWNERS.reduce( | ||
try { |
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.
Did some refactoring here, wrapped it all in a try/catch
so we can more easily identify errors when we're developing additional telemetry and now doing a promise.all
createdAt: savedObjects?.[0].saved_objects?.[0].attributes?.created_at ?? null, | ||
updatedAt: savedObjects?.[1].saved_objects?.[0].attributes?.updated_at ?? null, | ||
closedAt: savedObjects?.[2].saved_objects?.[0].attributes?.closed_at ?? null, | ||
createdAt: savedObjects?.[0]?.saved_objects?.[0]?.attributes?.created_at ?? '', |
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.
Prior to adding the ?
this was causing an exception if saved_objects[0]
was undefined which was skipping returning null
. Now that this doesn't throw an error I'm getting test failures because a keyword field can't be mapped to null 🤷♂️ .
Pinging @elastic/response-ops (Team:ResponseOps) |
Pinging @elastic/response-ops-cases (Feature:Cases) |
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.
Approving to unblock. I added a nit, though.
persistableAttachments: attachmentRegistrySchema, | ||
externalAttachments: attachmentRegistrySchema, | ||
files: { | ||
average: long, |
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.
Nit: do we want to keep the decimals in the avg? If so, we might need to mark it as double or float. WDYT?
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.
Great question, I was thinking integers since I don't know if care about the precision that much for this telemetry. I'll ask my PM though 👍
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 checked with my PM and we're going to keep them as longs.
💚 Build Succeeded
Metrics [docs]Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just 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.
LGTM! Thanks for the refactor. Much cleaner.
This PR adds telemetry for the attachment framework and files attachments by plugin and for all cases.
Issue: #151933
Notable changes:
Example telemetry payload
Testing
To test modify this file: https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/server/telemetry/schedule_telemetry_task.ts
With:
This will cause the telemetry to be sent as soon as the server is restarted.
To generate files and attachments to add stats to the telemetry I created this python script: https://github.com/elastic/cases-files-generator
To retrieve the telemetry: