-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[APM] Wrap Elasticsearch client errors #91125
Conversation
The stack trace from the ES client is not useful, as it loses the context of where the client was called. To get a useful stack trace, we create a new error. Additionally, we override message.toString() to see full ES error when logging to console.
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
.then((res) => res.body) | ||
.catch((err) => { | ||
// make sure stacktrace is relative to where client was called | ||
throw new WrappedElasticsearchClientError(err); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure you can use it for anything in this context but have you played around with Error.captureStackTrace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did, but that won't allow me to change the output of the error in the console. But I'll talk to some ops folks to see if we can address that issue for all errors rather than this hack.
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
@dgieselaar I think @elastic/kibana-core would be better equipped to help determine if there's a good place to log failures like this, we don't manage the loggers or ES client infrastructure used in the Kibana server. |
Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Dario Gieselaar <[email protected]>
* master: (157 commits) [DOCS] Adds machine learning to the security section of alerting (elastic#91501) [Uptime] Ping list step screenshot caption formatting (elastic#91403) [Vislib] Use timestamp on brush event instead of iso dates (elastic#91483) [Application Usage] Remove deprecated & unused legacy.appChanged API (elastic#91464) Migrate logstash, monitoring, url_drilldowns, xpack_legacy to ts projects (elastic#91194) [APM] Wrap Elasticsearch client errors (elastic#91125) [APM] Fix optimize-tsconfig script (elastic#91487) [Discover][docs] Add searchFieldsFromSource description (elastic#90980) Adds support for 'ip' data type (elastic#85087) [Detection Rules] Add updates from 7.11.2 rules (elastic#91553) [SECURITY SOLUTION] Eql in timeline (elastic#90816) [APM] Correlations Beta (elastic#86477) (elastic#89952) [Security Solutions][Detection Engine] Adds a warning banner when the alerts data has not been migrated yet. (elastic#90258) [Security Solution] [Timeline] Endpoint row renderers (2nd batch) (elastic#91446) skip flaky suite (elastic#91450) skip flaky suite (elastic#91592) [Security Solution][Endpoint][Admin] Endpoint Details UX Enhancements (elastic#90870) [ML] Add better UI support for runtime fields Transforms (elastic#90363) [Security Solution] [Detections] Replace 'partial failure' with 'warning' for rule statuses (elastic#91167) [Security Solution][Detections] Adds Indicator path config for indicator match rules (elastic#91260) ...
The stack trace from the ES client is not useful, as it loses the context of where the client was called. To get a useful stack trace, we create a new error. Additionally, we override message.toString() to see full ES error when logging to console, instead of only request metadata.
@elastic/kibana-operations I am wondering if there's a better way for the console output. Maybe the console output can be fixed for all errors? From what I can tell there's already a specific function to output errors - we can perhaps use
util.inspect(error, { depth: null })
there?Before:
After: