Skip to content

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

Merged
gnprice merged 3 commits intozulip:masterfrom
chrisbobbe:pr-event-logger
Jul 22, 2020
Merged

Improve long-loading logging code from the WebView.#4195
gnprice merged 3 commits intozulip:masterfrom
chrisbobbe:pr-event-logger

Conversation

@chrisbobbe
Copy link
Copy Markdown
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.

@chrisbobbe chrisbobbe requested a review from gnprice July 17, 2020 23:44
@gnprice
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants