feat: upgrade winston

[FauxFaux]

Requirements

• I have added test coverage for new or changed functionality
• I have followed the repository's pull request submission guidelines
• I have validated my changes against all supported platform versions

Describe the solution you've provided

Upgrade to a new major version of winston, the bundled logger.

Describe alternatives you've considered

Usage of logging here is relatively trivial; might be better suited to just exposing acceptance of a logging function directly.

[bwoskow-ld]

Hi @FauxFaux,

Thanks for the pull request!

The SDK team is curious -- what is your motivation for submitting this PR? Is there a particular Winston bug fix or new feature you're looking to take advantage of? You didn't mention anything in your PR description.

On general principle, the SDK team feels it is best to keep our dependency versions up to date for their latest bug fixes and improvements. If that's all you were aiming for, thanks again. We're just curious if there's something we should know about regarding the upgraded version. Note that regardless of Winston version used internally by our SDK, you're free to use a different version in your application.

Cheers,
Ben

[FauxFaux]

Heh, I appear to have not written down the plan anywhere. I was trying to shake some dependencies out of our transitive dependency tree, maybe async@1. It was not causing any runtime problem, but was showing up on an SCA report (licenses, deprecation, age).

[luiz290788]

This would also help me a lot for the same reasons @FauxFaux mentioned. Is there anything blocking this PR?

[eli-darkly]

@luiz290788: What was blocking it was that we hadn't noticed @FauxFaux's reply to @bwoskow-ld's question. Sorry - a few notifications fell through the cracks recently.

@FauxFaux: I don't see any reason not to accept this, but I do have one question about the PR description. You said: "Usage of logging here is relatively trivial; might be better suited to just exposing acceptance of a logging function directly". I'm not sure what "here" meant specifically, or why you would say it's "relatively trivial", but the SDK does use multiple log levels (debug, info, warn, error), which we consider to be fairly important functionality since people use it for alert filtering. So I'm not sure why you would want to use a simple function instead.

[FauxFaux]

Again, I'm afraid I don't remember why I wrote that. You do expose the ability to pass in a custom logger, and we are just doing that; and I suspect nearly everyone is; it's a node server application thing and most people will have logging.

The logging is "relatively trivial" in that our completely custom logger is a perfect drop-in replacement for your logger, at least, as far as we have noticed.

I think perhaps I was suggesting that the logger option could just be required, or default it to console (which also fulfils the contract); maybe nobody expects a library to provide its own logger. But, not really related to this PR, just an alternative.

[eli-darkly]

@FauxFaux - Got it. Well, changing it to default to console, unless we took the trouble to make the output appear in the exact same format that Winston uses, would be a change in behavior that could potentially be a breaking change for anyone who's currently capturing that output. Some of the other SDKs don't have a default for logging (that is, the default is none) and others do, so we'd have to think a bit more about what's best for the next major version.

[FauxFaux]

Yeah. Sticking with winston, so long as it works for you, is a perfectly fine choice here.

[eli-darkly]

Re-targeted this to new branch, will merge and pull into development branch for next release (which we should be able to put out fairly soon).

[eli-darkly]

Sorry, didn't mean to close this.

[eli-darkly]

Unfortunately, it looks like there was a problem with this, which I failed to see because (I think) the test code that I was running was left over from something else that actually substituted its own logger.

The code as written produces output like this:

{"message":"Initializing stream processor to receive feature flag updates","level":"info"}

I believe that's because the custom format function needs to be combined, instead of being used on its own:

// no
  format: prefixFormat()

// yes
  format: winston.format.combine(
    prefixFormat(),
    winston.format.simple()
  )

The release already went out but I'll have a patch shortly.

[eli-darkly]

OK, the 5.13.3 release is out now which should fix this