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

Improve long-loading logging code from the WebView. #4195

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

chrisbobbe
Copy link
Contributor

Prompted by Greg's suggestions at #4180 (comment).

There are a few differences to call out. If they're problematic, I'll fix them right away, of course!

The log-long-load state and all the logic referring to that state could be encapsulated in a class

Since I was making a class anyway, and putting it in its own JS file, I didn't want to entangle that class with the DOM detail of whether the message-loading div was visible. Instead, I made it so you can call "start" and "stop" methods on the class. I think this will also be useful if we want to log based on other, unrelated states. The code added in js.js is still much smaller than in the old way.

The map callback that produces loggableEvents could be pulled out and given a name like scrubUpdateEvent -- then logLongLoad would be much shorter.

Makes sense. I've pulled out the "scrubbing" logic into its own function, but the difference is: I don't use it in a map. As soon as the class instance becomes aware of the event, it scrubs it right away, before pushing it to the array.

@gnprice
Copy link
Member

gnprice commented Jul 21, 2020

Looks good! Two comments:

  • When at the 10-second mark we decide not to send, we should be sure to clear the array of log/event items. That way we don't hang on to those objects indefinitely when they'll never be used again.
  • The main disadvantage of scrubbing up front vs. scrubbing before send is that most of the time we don't end up logging anything, and in those cases it's a waste of work to bother scrubbing. But on the other hand we're not doing anything particularly expensive in this scrubbing -- it's not like we're searching through a big string or big object tree, we're just picking out some specific properties and in one case searching for one regexp -- so that's not a big deal. And it's also potentially nice to not hang onto the entire event with the entire content string -- in particular if there are a number of updates and so a number of copies of that string that we'd be keeping around until we decide whether to log. So it's likely this way is best in any case.

@chrisbobbe
Copy link
Contributor Author

chrisbobbe commented Jul 21, 2020

Sounds good! I just pushed my latest changes.

This reverts commit 523b307. In an upcoming commit, we'll replace
it with something more organized.
We're about to make another file that will help us get useful logs
from inside the WebView to Sentry. This lets us avoid an import
cycle; that new file and `js.js` both need to use `sendMessage`.
See 523b307 (reverted in a recent commit) for an earlier attempt
at this. It was done quickly to debug a problem with minimal delay;
this is the better-organized version of it, informed by Greg's
suggestions [1].

[1]: zulip#4180 (comment)
@gnprice gnprice merged commit 9b84434 into zulip:master Jul 22, 2020
@gnprice
Copy link
Member

gnprice commented Jul 22, 2020

Merged -- thanks!

@chrisbobbe chrisbobbe deleted the pr-event-logger branch November 6, 2020 03:20
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.

None yet

2 participants