-
Notifications
You must be signed in to change notification settings - Fork 850
Vendor part of Sentry-SDK for scrubbing requests errors #13434
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
Vendor part of Sentry-SDK for scrubbing requests errors #13434
Conversation
|
@rtibbles cc |
rtibbles
left a comment
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.
Generally, this is looking good to me. One small bit of cleanup where I think we can just use the KDS breakpoints.
I think we need to rebase the feature branch again, but easier to do that after this has been merged.
| import { browser, os, device, isTouchDevice } from 'kolibri.utils.browserInfo'; | ||
| import client from 'kolibri/client'; | ||
| import urls from 'kolibri/urls'; | ||
| import { browser, os, device, isTouchDevice, screenBreakpoint } from 'kolibri/utils/browserInfo'; |
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 think instead of adding this separate screenBreakpoint utility here, we can use the KDS breakpoints instead.
Can probably just import:
import useKResponsiveWindow from 'kolibri-design-system/lib/composables/useKResponsiveWindow';
then in the function body do:
const { windowBreakpoint } = useKResponsiveWindow();
Just need to make sure to then reference windowBreakpoint.value when passing it into the context object.
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 tried and had a problem:
windowBreakpoint.value is always null when i try to call it from kolibri/plugins/error_reports/assets/src/utils.js
Here is what I am doing
import useKResponsiveWindow from 'kolibri-design-system/lib/composables/useKResponsiveWindow';
const {windowBreakpoint} = useKResponsiveWindow();
console.log('windowBreakpoint', JSON.stringify(windowBreakpoint));
I see this in console:
windowBreakpoint {"value":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.
Hrm, this is probably because we're using it outside a component, so the tracking isn't working.
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.
So is there a workaround for this?
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 think it's probably easiest just to copy the logic and read the values when we report an error.
Can just copy the array here: https://github.com/learningequality/kolibri-design-system/blob/develop/lib/composables/useKResponsiveWindow/index.js#L67
And then compare the window width to get the breakpoint.
| "available_height": {"type": "integer", "optional": True}, | ||
| }, | ||
| }, | ||
| "screen_breakpoint": {"type": "string", "optional": True}, |
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.
If we're using the KDS breakpoints, then we can set this as a non-negative integer.
rtibbles
left a comment
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 is looking good to go, I'm going to give it a manual run through tomorrow to be sure, but nothing immediately jumping out to me!
| ] | ||
|
|
||
|
|
||
| def get_denylist(send_default_pii=False, custom_denylist=None): |
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 seems like we don't actually use this function anywhere? May be simpler to just merge the DEFAULT_PII_DENYLIST above into the other list, and then remove this function?
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.
Thanks! done
ab06242
into
learningequality:distributed-error-reporting
Summary
References
Fixes #13395
Reviewer guidance
NA