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

Remove dependency on deprecated domain module #528

Merged
merged 5 commits into from
Nov 20, 2023

Conversation

rg2011
Copy link
Contributor

@rg2011 rg2011 commented Nov 20, 2023

Fixes issue #498

domain module is deprecated since node 10 (https://nodejs.org/docs/latest-v10.x/api/domain.html). It is used to improve logging, by storing information related to the request in a per-request domain. The callback provided to function getContext of logops package queries that domain to retrieve request information.

This PR replaces the global logger with a per-request instance, stored in req.logger. This instance is created by a middleware that runs early in the chain, and populates the per-request logger instance with the same information that used to be in the context.

All the middleware and handlers are also refactored to use the per-request logger instead of the global one, so this PR can dispose of the logger.getContext callback and the domain dependency.

gavinlam and others added 2 commits November 18, 2023 14:02
fixed wrong variable

restored default values

remove obsolete library

Added missing curl needed in health checks

Updated curl doc
@rg2011
Copy link
Contributor Author

rg2011 commented Nov 20, 2023

Probably obsoletes #500 and #503

@AlvaroVega AlvaroVega self-requested a review November 20, 2023 09:08
Copy link
Member

@AlvaroVega AlvaroVega left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved
rg2011 and others added 2 commits November 20, 2023 10:44
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants