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

drop support for obsolete hapi package #2691

Closed
trentm opened this issue May 12, 2022 · 6 comments
Closed

drop support for obsolete hapi package #2691

trentm opened this issue May 12, 2022 · 6 comments
Assignees
Labels
agent-nodejs Make available for APM Agents project planning. breaking change
Milestone

Comments

@trentm
Copy link
Member

trentm commented May 12, 2022

To be clear, I'm talking about hapi, and not about dropping @hapi/hapi.

Starting with v17.9.0 and v18.2.0 the name changed from 'hapi' to '@hapi/hapi'.
hapi (the old one) is deprecated, obsolete, unmaintained and has known major security issues. IIUC it is coming up on two years being so. hapijs/hapi#4114 provides some details. Anything before @hapi/hapi@20 is in this "deprecated, ..." group.

% npm ci
npm WARN deprecated hapi@18.1.0: This version contains severe security issues and defects and should not be used! Please upgrade to the latest version of @hapi/hapi or consider a commercial license (https://github.com/hapijs/hapi/issues/4114)
...

There are maintenance and testing costs to continuing to support it. The monstrous .tav.yml block hints at some of the complexity:

apm-agent-nodejs/.tav.yml

Lines 408 to 464 in a289d44

# hapi and @hapi/hapi
# - Package name: Starting with v17.9.0 and v18.2.0 the name changed from
# 'hapi' to '@hapi/hapi'.
# - Node version compat:
# - hapi@15: supports node >=v4; breaks on node v14 (usage of `os.tmpDir()`)
# - hapi@16: supports node >=v4
# - hapi@17, @hapi/hapi@17: supports node >=v8.12.0 (per its README);
# the instrumentation changed significantly for this version
# - hapi@18, @hapi/hapi@18: supports node >=v8.12.0 (per its README)
# - @hapi/hapi@19: supports node >=v12 (judging from commit 50d8d7d)
# - @hapi/hapi@20: appears (from travis template refs) to support node >=v12
hapi-v9-v15:
name: hapi
versions: '>=9.0.1 <16.0.0'
node: '>=4 <14'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.test.js
- node test/instrumentation/modules/hapi/set-framework-hapi.test.js
hapi-v16:
name: hapi
versions: '>=16.0.0 <17.0.0'
node: '>=4'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.test.js
- node test/instrumentation/modules/hapi/set-framework-hapi.test.js
hapi-prenodev15:
name: hapi
versions: '>=17.0.0'
node: '>=8.12.0 <15.0.0'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.test.js
- node test/instrumentation/modules/hapi/set-framework-hapi.test.js
hapi:
name: hapi
# Work around https://github.com/npm/cli/issues/2267 in npm@7.
# Note: An alternative might be to just not test the "hapi" package with
# node >= 15, given that "hapi" was deprecated before node v16.
preinstall: rm -rf node_modules/hapi
node: '>=15.0.0'
versions: '>=17.0.0'
commands:
- node test/instrumentation/modules/hapi/basic-legacy-path.test.js
- node test/instrumentation/modules/hapi/set-framework-hapi.test.js
'@hapi/hapi-v17-v18':
name: '@hapi/hapi'
versions: '>=17.0.0 <19.0.0'
node: '>=8.12.0'
commands:
- node test/instrumentation/modules/hapi/basic.test.js
- node test/instrumentation/modules/hapi/set-framework-hapihapi.test.js
'@hapi/hapi':
name: '@hapi/hapi'
versions: '>=19.0.0'
node: '>=12'
commands:
- node test/instrumentation/modules/hapi/basic.test.js
- node test/instrumentation/modules/hapi/set-framework-hapihapi.test.js

Also, note there are lingering minor issues:

open questions

  • Do we drop it now, or do we wait for a major version bump of the agent?
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label May 12, 2022
@trentm
Copy link
Member Author

trentm commented May 12, 2022

  • which results in "extraneous" package entries in package-lock.json for npm versions before v8.6

I'm wrong about this being a "npm v8.6" difference. Using npm 8.10.0 locally I sometimes get the "extraneous" entries and sometimes not... depending on the exact npm install ... command used. Sigh.

@trentm
Copy link
Member Author

trentm commented May 12, 2022

Dependabot package-lock.json updates remove “extraneous” entries. Removing those entries results in npm install (with node v14’s npm v6) failing to install the ‘hapi’ package’s deps and tests fail. I think this is now the main motivator for dropping hapi. In a trade-off between testing old/obsolete 'hapi' vs. using dependabot for security and dep updates, I'll take dependabot.

@trentm trentm self-assigned this May 12, 2022
@trentm
Copy link
Member Author

trentm commented May 17, 2022

In the team meeting, @estolfo suggested dropping testing of the obsolete "hapi" package for now, and defer dropping actual support for it until the next major. I think that sounds good. I will work on a PR to try that. If that works, I'll put this issue on the "next-major" milestone.

trentm added a commit that referenced this issue May 18, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
The old 'hapi' package was itself deprecated about 2 years ago.  The APM
agent now deprecates instrumenting it. The instrumentation code remains,
but it is no longer tested and will be removed in the next major.

Note that '@hapi/hapi' is still supported, this is just about the old
package name and versions. Any versions starting with 17.9.0, 18.2.0,
19 and later are the newer '@hapi/hapi'.

Refs: #2691
@trentm trentm added this to the next-major milestone May 18, 2022
@estolfo
Copy link
Contributor

estolfo commented May 19, 2022

And just make sure you document that the obsolete hapi package support is untested and deprecated so that users know they are using its instrumentation it at their own risk.

@trentm
Copy link
Member Author

trentm commented May 19, 2022

And just make sure you document that the obsolete hapi package support is untested and deprecated so that users know they are using its instrumentation it at their own risk.

Yup, that was done both in the changelog (https://github.com/elastic/apm-agent-nodejs/pull/2698/files#diff-cb0601935fcaac3fea29b215282428b964b8b6e1bd5a6b8c68e84d651633c9e3) and supported-technologies doc page (https://github.com/elastic/apm-agent-nodejs/pull/2698/files#diff-d2a92236665ca681fc14a4b85cb60e74bf3d25b42328ca2390fca53156100d25) of #2698

astorm pushed a commit that referenced this issue May 19, 2022
The old 'hapi' package was itself deprecated about 2 years ago.  The APM
agent now deprecates instrumenting it. The instrumentation code remains,
but it is no longer tested and will be removed in the next major.

Note that '@hapi/hapi' is still supported, this is just about the old
package name and versions. Any versions starting with 17.9.0, 18.2.0,
19 and later are the newer '@hapi/hapi'.

Refs: #2691
@trentm trentm removed their assignment May 24, 2022
trentm added a commit that referenced this issue Aug 2, 2023

Verified

This commit was signed with the committer’s verified signature.
trentm Trent Mick
Closes: #2691
@trentm trentm self-assigned this Aug 2, 2023
trentm added a commit that referenced this issue Aug 3, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Closes: #2691
@trentm
Copy link
Member Author

trentm commented Aug 3, 2023

Resolved by #3541 on the "dev/4.x" branch, which will be the 4.x active branch soonish.

@trentm trentm closed this as completed Aug 3, 2023
trentm added a commit that referenced this issue Sep 1, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
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)
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this issue Aug 20, 2024
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
Labels
agent-nodejs Make available for APM Agents project planning. breaking change
Projects
None yet
Development

No branches or pull requests

2 participants