DEVPROD-356: Add Sentry grouping #2262
Conversation
3 flaky tests on run #15784 ↗︎
Details:
Review all test suite changes for PR #2262 ↗︎ |
||||||||||||||||||||
sophstad
force-pushed
the
DEVPROD-356
branch
from
1a7b68b to
e5f39d6
Compare
| @@ -46,6 +47,21 @@ const sendError = ( | |||
| withScope((scope) => { | |||
| setScope(scope, { level: severity, context: metadata }); | |||
|
|
|||
| const { gqlErr, operationName } = metadata ?? {}; | |||
There was a problem hiding this comment.
Can we improve the type safety on these by adding gqlErr and operationName as option fields in the metadata struct on line 46.
| scope.setFingerprint(fingerprint); | ||
|
|
||
| // Apply tag, which is a searchable/filterable property | ||
| setTag("operationName", metadata.operationName); |
There was a problem hiding this comment.
| setTag("operationName", metadata.operationName); | |
| setTag("operationName", operationName); |
|
Can we also make these changes in parsley |
| const sendError = ( | ||
| err: Error, | ||
| severity: SeverityLevel, | ||
| metadata?: { [key: string]: any }, | ||
| metadata?: ErrorMetadata, |
There was a problem hiding this comment.
This typing is technically broken since
spruce/src/utils/errorReporting.ts
Line 10 in 45550f1
{ [key:string]: any } so this type is never actually used.
There was a problem hiding this comment.
It is also not flexible enough to allow for any metadata to be passed in once the typing in reportError is fixed.
There was a problem hiding this comment.
Updated the typing in errorReporting.ts. If you want strict typing I think we should just go all the way and update this type as more metadata fields are added rather than allow the type to be flexible.
| gqlErr?: ErrorResponse["graphQLErrors"][0]; | ||
| operationName?: ErrorResponse["operation"]["operationName"]; | ||
| variables?: ErrorResponse["operation"]["variables"]; |
There was a problem hiding this comment.
These are all actually typed from
import { GraphQLError } from "graphql"; since they are properties of the GraphQL request and not specific to Apollo.
There was a problem hiding this comment.
This is the correct typing of Apollo's onError handlers if I'm not mistaken: https://github.com/apollographql/apollo-client/blob/17ccc42825d0b0355212f3ac823780edb467ea77/src/link/error/index.ts#L28
GraphQLError doesn't include operationName or variables: https://github.com/graphql/graphql-js/blob/9c90a23dd430ba7b9db3d566b084e9f66aded346/src/error/GraphQLError.ts#L41
These are Apollo-specific since they come from Apollo links; the GraphQL request is just a part of the data in the graphQLErrors field.
|
Can we do this on Parsley as well please! @sophstad |
DEVPROD-356
Description
Added two new fields to GraphQL errors that should help with grouping in the app:
Testing
UserProjectSettingsPermissionsbeing thrown with an error in the project identifier.operationNameis now a tag appearing at the top of the page:I think this is a pretty logical way of grouping things. You can also take a look on staging and see how incoming errors are grouped. Sentry's "recommended error" view is still confusing to me, and the error titles are still somewhat useless, but I think the grouping is improved.