Skip to content
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

refactor: improve error handling during ClickHouse query type mismatch while filtering session replays #24300

Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { lemonToast } from '@posthog/lemon-ui'
import { captureException } from '@sentry/react'
import equal from 'fast-deep-equal'
import { actions, afterMount, connect, kea, key, listeners, path, props, propsChanged, reducers, selectors } from 'kea'
import { loaders } from 'kea-loaders'
Expand Down Expand Up @@ -359,7 +361,20 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
await breakpoint(400) // Debounce for lots of quick filter changes

const startTime = performance.now()
const response = await api.recordings.list(params)
let response

try {
response = await api.recordings.list(params)
} catch (error: any) {
captureException(error)
actions.loadSessionRecordingsFailure(error.message, error)

return {
has_next: false,
results: [],
}
}
Comment on lines +381 to +391
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the exception isn't handled in this code section, it will be processed by the loadersPlugin in initKea.ts. This will trigger the onFailure function, which already includes error handling with lemonToast.error, resulting in a duplicate error toast.

For reference, here’s the relevant code:

if (errorMessage) {
lemonToast.error(`${identifierToHuman(actionKey)} failed: ${errorMessage}`)
}

I’ve added exception handling above to prevent the duplication of error toasts.


const loadTimeMs = performance.now() - startTime

actions.reportRecordingsListFetched(loadTimeMs, values.filters, defaultRecordingDurationFilter)
Expand Down Expand Up @@ -580,6 +595,17 @@ export const sessionRecordingsPlaylistLogic = kea<sessionRecordingsPlaylistLogic
actions.maybeLoadSessionRecordings('older')
}
},

loadSessionRecordingsFailure: ({ error, errorObject }) => {
if (errorObject?.data?.type === 'validation_error' && errorObject?.data?.code === 'type_mismatch') {
lemonToast.error(
'One or more property values in your filters are in an incorrect format.' +
' Please check and adjust them to match the required data types.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the perfect thing here would be for the toast to link to the data management properties page... iirc we can set a button on the toast so the person has an onward journey

(even better if it's to the specific property)

)
} else {
lemonToast.error(`Error loading session recordings: ${error}`)
}
},
})),
selectors({
logicProps: [() => [(_, props) => props], (props): SessionRecordingPlaylistLogicProps => props],
Expand Down
8 changes: 7 additions & 1 deletion posthog/session_recordings/session_recording_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@
from rest_framework.renderers import JSONRenderer
from rest_framework.response import Response
from rest_framework.utils.encoders import JSONEncoder
from rest_framework.exceptions import ValidationError

from posthog.api.person import MinimalPersonSerializer
from posthog.api.routing import TeamAndOrgViewSetMixin
from posthog.api.utils import safe_clickhouse_string
from posthog.auth import SharingAccessTokenAuthentication
from posthog.cloud_utils import is_cloud
from posthog.constants import SESSION_RECORDINGS_FILTER_IDS
from posthog.errors import ExposedCHQueryError
from posthog.hogql.errors import ExposedHogQLError
from posthog.models import User, Team
from posthog.models.filters.session_recordings_filter import SessionRecordingsFilter
from posthog.models.person.person import PersonDistinctId
Expand Down Expand Up @@ -276,7 +279,10 @@ def safely_get_object(self, queryset) -> SessionRecording:

def list(self, request: request.Request, *args: Any, **kwargs: Any) -> Response:
filter = SessionRecordingsFilter(request=request, team=self.team)
return list_recordings_response(filter, request, self.get_serializer_context())
try:
return list_recordings_response(filter, request, self.get_serializer_context())
except (ExposedHogQLError, ExposedCHQueryError) as e:
raise ValidationError(str(e), getattr(e, "code_name", None))
Copy link
Contributor Author

@namit-chandwani namit-chandwani Aug 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the section that raises ValidationError because I believe ExposedHogQLError and ExposedCHQueryError are client-side errors. Therefore, a 4xx HTTP status code, particularly 400 (bad request), seems more fitting than the 500 (internal server error) status code that was originally being used.

Please let me know if this assumption is incorrect and if I should adjust or remove this implementation

Dismissed Show dismissed Hide dismissed

@extend_schema(
description="""
Expand Down