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.

Sign up for GitHub

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

Jump to bottom

remove logUncaughtExceptions config var and print uncaught exceptions to stderr by default #2412

Closed
trentm opened this issue Nov 3, 2021 · 2 comments
Closed

remove logUncaughtExceptions config var and print uncaught exceptions to stderr by default #2412

trentm opened this issue Nov 3, 2021 · 2 comments
Assignees
@trentm
Labels
agent-nodejs Make available for APM Agents project planning.
Milestone
next-major

Comments

@trentm
Copy link
Member

trentm commented Nov 3, 2021 •
edited
Loading

By default the APM agent installs an uncaughtException handler to send an error to APM server. By default, so as to not change application behaviour, it exits the process after sending the error.

A side-effect of installing an uncaughtException handler is that node core no longer does its full default behaviour if there are no such handlers. Its default behaviour is to (a) print the error to stderr and (b) exit (modulo --abort-on-uncaught-exception).

If the logUncaughtExceptions: true config option is given to the APM agent, then it will also print the error to stderr.
https://www.elastic.co/guide/en/apm/agent/nodejs/current/configuration.html#log-uncaught-exceptions
logUncaughtExceptions defaults to false. I think it should default to true.

Note #2367 as well. If we switch to using uncaughtExceptionMonitor (in node v12.7.0 and greater) and that solves this issue, then great. Perhaps, then, this issue will only be relevant for earlier node versions.

Update 2023-08-08: Using uncaughtExceptionMonitor is promising but too much work for now. We can just drop the logUncaughtExceptions config option instead, and print a captured uncaught exception to stderr by default.
@trentm trentm added this to the next-major milestone Nov 3, 2021
@trentm trentm changed the title logUncaughtException should default to true logUncaughtExceptions should default to true Nov 3, 2021
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Nov 3, 2021
@trentm trentm mentioned this issue Nov 4, 2021
Closed
3 tasks
@trentm trentm mentioned this issue Jul 26, 2022
Closed
@trentm
Copy link
Member Author

trentm commented Aug 8, 2023

Note #2367 as well. If we switch to using uncaughtExceptionMonitor (in node v12.7.0 and greater) and that solves this issue, then great. Perhaps, then, this issue will only be relevant for earlier node versions.

See #2367 (comment)
tl;dr: I think switching to using uncaughtExceptionMonitor -- instead of uncaughtException -- would be a good thing eventually, but not now for the 4.0.0 milestone because of development effort involved.

@trentm trentm self-assigned this Aug 8, 2023
@trentm trentm changed the title logUncaughtExceptions should default to true remove logUncaughtExceptions config var and print uncaught exceptions to stderr by default Aug 9, 2023
@trentm trentm mentioned this issue Aug 9, 2023
Merged
trentm added a commit that referenced this issue Aug 14, 2023
@trentm
feat!: remove logUncaughtExceptions config var and print uncaught e…
23ea8ec 
…xceptions to stderr by default (#3570)

Closes: #2412
@trentm
Copy link
Member Author

trentm commented Aug 14, 2023

Closed by #3570 on the "dev/4.x" branch, which will become the 4.x release branch soonish.

@trentm trentm closed this as completed Aug 14, 2023
trentm added a commit that referenced this issue Sep 1, 2023
@trentm
4.x (#3528)
c6cdaf9 
feat!: the start of v4.x

Significant and breaking changes:
- Min supported Node.js is now 14.5.0 (up from 8.6).
- Drop support for the obsolete "patch" context manager, i.e. the
  `contextManager: "patch"` config option. This was a limited async context
  management that predated the preferred `AsyncLocalStorage` core Node.js
  mechanism for context tracking. (#3529)
- Config vars:
    - Remove `logUncaughtExceptions` config option, if the agent's
      `uncaughtException` handler is active it now *always* logs the error to
      the console. (#2412)
    - Remove `filterHttpHeaders` config option, see `sanitizeFieldNames`
      instead. (#3332)
    - Remove long deprecate `ELASTIC_APM_KUBERNETES_*` envvars, use
      `KUBERNETES_*` instead. (#2661)
    - The `useElasticTraceparentHeader` config option now defaults to `false`.
      This means the `elastic-apm-traceparent` HTTP header is now no longer
      sent by default. (#3555)
    - Drop erroneous `ELASTIC_SANITIZE_FIELD_NAMES` and
      `ELASTIC_IGNORE_MESSAGE_QUEUES` config envvars.
- Instrumentations:
    - Drop instrumentation for old `hapi`, the current `@hapi/hapi` is still
      instrumented. (#2691)
- APIs:
    - Ignore a `timer` option passed to `startTransaction()` and `startSpan()`
      APIs. (#2990)
    - Remove the deprecated `span.toString()` and `transaction.toString()` APIs.
      (#2348)
    - Change `apm.startTransaction()` API to return a noop transaction instead
      of null when the agent is not started. (#2429)
    - Remove `transaction.subtype` and `transaction.action` properties from the
      API. This also impacts <<apm-start-transaction>> and
      `transaction.setType(...)`, both of which now no longer accept `subtype`
      and `action` parameters. (#3557)
fpm-peter pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
@trentm @fpm-peter
4.x (elastic#3528)
52924ec 
feat!: the start of v4.x

Significant and breaking changes:
- Min supported Node.js is now 14.5.0 (up from 8.6).
- Drop support for the obsolete "patch" context manager, i.e. the
  `contextManager: "patch"` config option. This was a limited async context
  management that predated the preferred `AsyncLocalStorage` core Node.js
  mechanism for context tracking. (elastic#3529)
- Config vars:
    - Remove `logUncaughtExceptions` config option, if the agent's
      `uncaughtException` handler is active it now *always* logs the error to
      the console. (elastic#2412)
    - Remove `filterHttpHeaders` config option, see `sanitizeFieldNames`
      instead. (elastic#3332)
    - Remove long deprecate `ELASTIC_APM_KUBERNETES_*` envvars, use
      `KUBERNETES_*` instead. (elastic#2661)
    - The `useElasticTraceparentHeader` config option now defaults to `false`.
      This means the `elastic-apm-traceparent` HTTP header is now no longer
      sent by default. (elastic#3555)
    - Drop erroneous `ELASTIC_SANITIZE_FIELD_NAMES` and
      `ELASTIC_IGNORE_MESSAGE_QUEUES` config envvars.
- Instrumentations:
    - Drop instrumentation for old `hapi`, the current `@hapi/hapi` is still
      instrumented. (elastic#2691)
- APIs:
    - Ignore a `timer` option passed to `startTransaction()` and `startSpan()`
      APIs. (elastic#2990)
    - Remove the deprecated `span.toString()` and `transaction.toString()` APIs.
      (elastic#2348)
    - Change `apm.startTransaction()` API to return a noop transaction instead
      of null when the agent is not started. (elastic#2429)
    - Remove `transaction.subtype` and `transaction.action` properties from the
      API. This also impacts <<apm-start-transaction>> and
      `transaction.setType(...)`, both of which now no longer accept `subtype`
      and `action` parameters. (elastic#3557)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@trentm trentm

Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Milestone
next-major
Development

No branches or pull requests
1 participant
@trentm