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

Attempt to reduce noise with isUnsupportedBrowser #14979

Merged
merged 1 commit into from May 10, 2017
Merged

Conversation

caleybrock
Copy link
Contributor

Our only guess right now is that this could be a timing thing, since it only happens sometimes.
We can definitely reduce noise by checking if it's on window first, but this represents a real error with how we include common js in pegasus so I don't want to totally silence it. @Bjvanminnen's idea was to add a timeout before making the check to see if it eventually gets added to window.

@Bjvanminnen
Copy link
Contributor

So if this is successful in reducing our error rate, that means we almost certainly have a timing issue that we don't fully understand or think we should have. At that point, I don't know that we should consider this issue fixed.

If this does not reduce our error rate, I believe it implies our misunderstanding is either somewhere else, or we have a timing issue where the window is > 5s.

@caleybrock
Copy link
Contributor Author

I'll also note here that I tested user impact. The only case where this impacts the user, is if they are on an unsupported browser, and have this timing issue, they won't see a banner telling them that their browser is unsupported. If they get the error on a supported browser, there is no impact.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

This is fine, but why haven't we just moved this $(document).ready code out of the haml files and into whatever bundle is providing isUnsupportedBrowser in the first place? It's not clear to me that it ever needs to be on window at all.

@@ -5,7 +5,15 @@

:javascript
$(document).ready(function () {
if (!window.isUnsupportedBrowser) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a really small nit, but I would have written:

setTimeout(() => {
  if (isUnsupportedBrowser()) {
    $('#warning-banner').show();
  }
}, window.isUnsupportedBrowser ? 0 : 5000);

...which doesn't have quite the same meaning (because it introduces a setTimeout(_, 0)) but in this case might be good enough, and might be easier to reason about (e.g. we will always run this callback, it's just a question of when).

Copy link
Contributor

Choose a reason for hiding this comment

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

One downside to that approach Brad is that in the case where isUnsupportedBrowser is undefined, we won't be able to tell from the callstack in NR whether it's on the initial load or on the delayed load.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't envision the current approach as a long term solution, but rather a short term solution that (a) reduces noise and (b) provides us with some additional information about what's failing (i.e. is it actually a timing issue or something else) that will hopefully make it easier for us to find a long term solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is a good point about different callstacks.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

LGTM as a way to gather more data. The timing issue seems like it would be hard to debug, so I like that this will help us determine if timing is really part of the problem.

@caleybrock caleybrock merged commit ca15e41 into staging May 10, 2017
@caleybrock caleybrock deleted the pegasus-errors branch May 10, 2017 19:28
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

4 participants