-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(ourlogs): Add error event to log list #103649
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
Conversation
On issues details, this will add the error event into the logs list to give context as to where the error happened in time.
| __isPseudoRow: true, | ||
| __originalEvent: event, | ||
| } as PseudoLogResponseItem; | ||
| } |
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.
Bug: Missing ORGANIZATION_ID in pseudo log response item
The PseudoLogResponseItem interface requires ORGANIZATION_ID as a mandatory number field, but the createPseudoLogResponseItem function doesn't provide this field when creating the object. This could cause runtime issues where code expects the field to exist but finds it missing. The function should either accept an organizationId parameter or extract it from the event object.
| const eventMoment = moment(eventTimestamp); | ||
| const start = getUtcDateString(eventMoment.clone().subtract(1, 'day')); | ||
| const end = getUtcDateString(eventMoment.clone().add(1, 'day')); | ||
| const environment = getEventEnvironment(event); |
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.
Could this create an end timestamp in the future? Does the time selector handle that gracefully?
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 does create an end?
| const timestamps = data | ||
| .filter((row): row is OurLogsResponseItem => isRegularLogResponseItem(row)) | ||
| .map(row => getLogRowTimestampMillis(row)) | ||
| .filter(Boolean); |
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.
Can this just map over baseData?
| return -1; | ||
| } | ||
| const event = additionalData.event; | ||
| const eventTimestamp = new Date(event.dateCreated || new Date()).getTime(); |
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.
Bug: Pseudo row positioned incorrectly if event lacks dateCreated
When calculating where to insert the pseudo row, the code falls back to the current time (new Date()) if event.dateCreated is missing. This means events without a dateCreated will always appear at or near the end of the log list, regardless of their actual timestamp. The code should use event.dateReceived as a fallback like the drawer does, or handle missing timestamps more gracefully.
On issues details, this will add the error event into the logs list to give context as to where the error happened in time.