Skip to content

Comments

Don't hide errors inside app.mount fn#66764

Merged
stacey-gammon merged 2 commits intoelastic:masterfrom
stacey-gammon:2020-05-15-add-console-error
May 18, 2020
Merged

Don't hide errors inside app.mount fn#66764
stacey-gammon merged 2 commits intoelastic:masterfrom
stacey-gammon:2020-05-15-add-console-error

Conversation

@stacey-gammon
Copy link

Just got bit by this. Don't swallow errors, at least console.error them.

@stacey-gammon stacey-gammon changed the title dont hide errors Don't hide errors inside app.mount fn May 15, 2020
@stacey-gammon stacey-gammon added release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0 labels May 15, 2020
@stacey-gammon stacey-gammon requested a review from joshdover May 15, 2020 16:31
@stacey-gammon stacey-gammon marked this pull request as ready for review May 15, 2020 16:31
@stacey-gammon stacey-gammon requested a review from a team as a code owner May 15, 2020 16:31
Comment on lines +90 to +91
// eslint-disable-next-line no-console
console.error(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

@joshdover Mixed about whether this is acceptable until we actually implements the TODO or not. We should probably at least wrap this in if process.env === 'development', wdyt ?

Copy link
Contributor

Choose a reason for hiding this comment

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

We actually just had an SDH where a customer could not see a dashboard because the app failed to mount on a corrupted saved object. Having a stack trace from production would have been a huge help. I'm ok with keep this for now until we get a UI

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM. We really need to implement client-side logging API at some point to avoid these console.log

@stacey-gammon
Copy link
Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stacey-gammon stacey-gammon merged commit 3b4814b into elastic:master May 18, 2020
stacey-gammon pushed a commit to stacey-gammon/kibana that referenced this pull request May 18, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 19, 2020
* master: (24 commits)
  [APM] agent config 'profiling_inferred_spans_min_duration' default value is '0ms' but the min value in the field is '1ms' (elastic#66886)
  [Canvas] Fix flaky custom element functional tests (elastic#65908)
  Fix IE specific flexbox min-height issue (elastic#66555)
  [Discover] Unskip doc link functional test (elastic#66884)
  Index pattern management to Kibana platform (elastic#65026)
  Warning and link to support matrix for IE11 (elastic#66512)
  [Reporting] Consolidate Server Type Defs, move some out of Legacy (elastic#66144)
  [SIEM] [Maps] Fixes Network Map empty tooltip (elastic#66828)
  [Endpoint] Encode the index of the alert in the id response (elastic#66919)
  [services/testSubjects] reduce retry usage, add waitForEnabled (elastic#66538)
  [DOCS] Identifies cloud settings for APM (elastic#66935)
  [SIEM][CASE] Fix configuration's page user experience (elastic#66029)
  Resolver: Display node 75% view submenus (elastic#64121)
  [SIEM] Cases] Capture timeline click and open timeline in case view (elastic#66327)
  [APM] Lowercase agent names so icons work (elastic#66824)
  [dev/cli] add support for --no-cache (elastic#66837)
  [Ingest Manager] Better handling of package installation problems (elastic#66541)
  [ML] Enhances api docs for modules endpoints (elastic#66738)
  dont hide errors (elastic#66764)
  [RFC] Global search API (elastic#64284)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 20, 2020
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

stacey-gammon pushed a commit that referenced this pull request May 20, 2020
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes v7.9.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants