Streaming: replace npmlog with pino & pino-http

[ThisIsMissEm]

This implements #27387.

Advice for reviewers: I'd recommend going commit by commit in the files changed tab, as each commit tries to focus in on specific changes (though the change from verifyClient to server.on("upgrade") is bundled with the logger change)

In order to properly initialize the logging, I had to rework the WebSocket Server attachment to the HTTP server to use the upgrade event, which allows for making a decision around authentication before even attempting to upgrade the connection, which is preferrable to verifyClient whose usage is now discouraged.

By using the upgrade event and ws.handleUpgrade method allows us to pass through a logger to the wss.on("connection") event/handler, which can be augmented with logging details specific to websockets.

The log redaction and sanitizer is a little complicated, but essentially prevents access tokens from accidentally ending up in server logs.

The request.id is now managed by the logger, and we don't need request.remoteAddress as the request logger correctly captures the information about the request for logging, so we're able to remove both of those middlewares. Additionally, I replaced the custom CORS middleware with the standard and widely used cors package, which is well tested.

Bundled in is the start of some code cleanup, moving things out to separate files where appropriate (since these things where making it much more difficult to read the code whilst I changed the connection upgrade handling).

In development, if you want, you can pipe to pino-pretty for nicer looking logs, which is available via a development dependency:

$ yarn workspace @mastodon/streaming start | pino-pretty

[ThisIsMissEm]

@renchap 9b24627 is what I was talking about over on #24702, it didn't directly break anything when linting ran, but did cause issues; This will also act as a starting point for eventually being able to migrate to ESM, without affecting the reset of the codebase.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[mjankowski]

This is definitely easier to process one commit at a time than all together, but even so, it seems like at least:

• The eslint/tsconfig changes
• Extracting the metrics to a separate file
• Move isTruthy to utils
• Swap out custom impl for cors package

...could all be stand-alone PRs (they all seem like improvements), and may make it easier to review/merge the narrower stated purpose here of the logger change?

[ThisIsMissEm]

@mjankowski I've tried in the past to split changes down, but due to the project merge cadence I had to abandon those PRs which were all stacked and a nightmare to maintain / rebase.

I'm trying to avoid that here.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[ThisIsMissEm]

Extracted the CORS change to #28523 & rebased this PR to separate the server initialization changes from the logging changes as best as possible.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[renchap]

Now that the CORS refactor is merged, could you extract your websocket refactor commit into it's own PR?

Then I think we will be able to review & merge this :)

Thanks a lot for this work!

[ThisIsMissEm]

Frustrated reply when sick

Now that the CORS refactor is merged, could you extract your websocket refactor commit into it's own PR?

Then I think we will be able to review & merge this :)

Thanks a lot for this work!

With all due respect, this is the fourth time I've done the work in this branch & tried to get it merged. I've had to abandon 3 other attempts. My time, like yours is extremely limited. I have tried to make the changes reviewable commit by commit:
https://github.com//pull/27828/commits/6533a523d37e5cf5afb7ec012409f141f2064e33
https://github.com//pull/27828/commits/93351e1c6c865c281687dfd190720246de58b8bb

I do not have time or energy to continuously rebase this work, nor do I have the energy as I'm currently reasonably unwell. You either want this change and for the streaming server to improve, or you don't. I know your resources are thin, but mine are likely thinner. I've been trying to get this work merged for almost a year at this point.

Edit: I've hidden this reply because it's not normally how I like to conduct myself; I was incredibly tired, sick and overwhelmed with a bunch of things.

[ThisIsMissEm]

There is a follow-on related to this pull request here: #28632

[ThisIsMissEm]

@ClearlyClaire @mjankowski okay, I think this should be good to go; I've moved the postgresql and redis type issues into a separate PR (still to finish, because I want to move those out of index.js).

[ClearlyClaire]

Apart from the minor naming issues, this looks fine to me, but I'd like another set of eyes on this.

Also, this is a (fairly minor) breaking change that needs to be documented.

[github-actions]

This pull request has merge conflicts that must be resolved before it can be merged.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bd53859) 84.85% compared to head (33d0bb8) 84.86%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #27828   +/-   ##
=======================================
  Coverage   84.85%   84.86%           
=======================================
  Files        1038     1038           
  Lines       28162    28162           
  Branches     4531     4531           
=======================================
+ Hits        23898    23899    +1     
  Misses       3103     3103           
+ Partials     1161     1160    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[github-actions]

This pull request has resolved merge conflicts and is ready for review.

[ThisIsMissEm]

Also, this is a (fairly minor) breaking change that needs to be documented.

@ClearlyClaire currently the documentation site has absolutely zero mention of logging, and only a little bit on monitoring Mastodon. I did add some documentation about streaming's metrics to the "scaling up" docs, but I don't see a good place for the notice that this is a breaking change in how the streaming server produces logs.

By the looks the changelog should be updated in the release branch.

[ClearlyClaire]

Also, this is a (fairly minor) breaking change that needs to be documented.

@ClearlyClaire currently the documentation site has absolutely zero mention of logging, and only a little bit on monitoring Mastodon. I did add some documentation about streaming's metrics to the "scaling up" docs, but I don't see a good place for the notice that this is a breaking change in how the streaming server produces logs.

By the looks the changelog should be updated in the release branch.

Yes, I meant that as a note to add something in the release notes when the time comes, not as something to do before the PR gets merged.

[renchap]

Looks good to me. Quite a hard change to test entirely, but it makes the code cleaner and hopefully provides more useful logs.