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

add logging when a request is being cancelled #1195

Merged
merged 1 commit into from
Feb 4, 2020

Conversation

bwindels
Copy link
Contributor

@bwindels bwindels commented Feb 4, 2020

so we can more easily see (especially for to_device requests)
why something was cancelled, to debug issues like element-hq/element-web#12226

so we can more easily see (especially for to_device requests)
why something was cancelled
@bwindels bwindels requested a review from a team February 4, 2020 11:48
// requests on its own
if (type === CANCEL_TYPE) {
logger.log(`verification request ${this.channel.transactionId}: ` +
`.cancel event with ${JSON.stringify(event.getContent())} ` +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe pass this as a separate not-stringifed arg so it's easier to inspect in the console?

Copy link
Contributor Author

@bwindels bwindels Feb 4, 2020

Choose a reason for hiding this comment

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

logger.log doesn't support multiple arguments I think? Also, if it did/does, I'm not sure how that would appear in the logs sent through rageshakes. Not [Object object] or something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine to merge either way. console.log definitely does, so we might want to change the logger in some other PR. Rageshakes already stringify where needed.

@bwindels bwindels merged commit ee4c6b6 into develop Feb 4, 2020
@t3chguy t3chguy deleted the bwindels/logoncancel branch May 10, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants