-
Notifications
You must be signed in to change notification settings - Fork 1.1k
(feat) Crashlytics tools improvements #9138
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import { logger } from "../logger"; | ||
import { FirebaseError, getError } from "../error"; | ||
import { CRASHLYTICS_API_CLIENT, parseProjectNumber, TIMEOUT } from "./utils"; | ||
import { ListEventsResponse } from "./types"; | ||
import { BatchGetEventsResponse, ListEventsResponse } from "./types"; | ||
import { EventFilter, filterToUrlSearchParams } from "./filters"; | ||
|
||
/** | ||
|
@@ -43,3 +43,44 @@ export async function listEvents( | |
}); | ||
} | ||
} | ||
|
||
/** | ||
* Get multiple events by resource name. | ||
* Can be used with the `sampleEvent` resource included in topIssues reports. | ||
* @param appId Firebase app_id | ||
* @param eventNames the resource names for the desired events. | ||
* Format: "projects/{project}/apps/{app_id}/events/{event_id}" | ||
* @return A BatchGetEventsResponse including an array of Event. | ||
*/ | ||
export async function batchGetEvents( | ||
appId: string, | ||
eventNames: string[], | ||
): Promise<BatchGetEventsResponse> { | ||
const requestProjectNumber = parseProjectNumber(appId); | ||
if (eventNames.length > 100) throw new FirebaseError("Too many events in batchGet request"); | ||
maxl0rd marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we even let this many come through? I feel like we shouldn't pull back more than like 30 tops to keep the context window from blowing up. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't written the tool yet, but I think we could limit it there. These defs map to the api's limits. |
||
logger.debug( | ||
`[crashlytics] batchGetEvents called with appId: ${appId}, eventNames: ${eventNames.join(", ")}`, | ||
); | ||
const queryParams = new URLSearchParams(); | ||
eventNames.forEach((en) => { | ||
queryParams.append("event.names", en); | ||
}); | ||
|
||
try { | ||
const response = await CRASHLYTICS_API_CLIENT.request<void, BatchGetEventsResponse>({ | ||
method: "GET", | ||
headers: { | ||
"Content-Type": "application/json", | ||
}, | ||
path: `/projects/${requestProjectNumber}/apps/${appId}/events:batchGet`, | ||
queryParams: queryParams, | ||
timeout: TIMEOUT, | ||
}); | ||
|
||
return response.body; | ||
} catch (err: unknown) { | ||
throw new FirebaseError(`Failed to batch get events for app_id ${appId}.`, { | ||
original: getError(err), | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,8 +14,14 @@ export const IssueIdSchema = z.string().describe("Crashlytics issue id, as hexid | |
|
||
export const EventFilterSchema = z | ||
.object({ | ||
intervalStartTime: z.string().optional().describe(`A timestamp in ISO 8601 string format`), | ||
intervalEndTime: z.string().optional().describe(`A timestamp in ISO 8601 string format.`), | ||
intervalStartTime: z | ||
.string() | ||
.optional() | ||
.describe(`A timestamp in ISO 8601 string format. Defaults to 7 days ago.`), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add the default value to zod? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We wouldn't want a static timestamp burned into the tool definition. |
||
intervalEndTime: z | ||
.string() | ||
.optional() | ||
.describe(`A timestamp in ISO 8601 string format. Defaults to now.`), | ||
versionDisplayNames: z | ||
.array(z.string()) | ||
.optional() | ||
|
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 looks like we don't have a tool defined for this - do we want to use this now? Should we keep it out of here until we find we have a need for it?
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.
This should be fine to add the library method here, but I want to do more testing before adding the tool.