launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

How to use other loggers? #196

Closed rektide closed 3 years ago

rektide commented 4 years ago

Is your feature request related to a problem? Please describe.

All of our systems use the Pino logger. Not particularly important, but it's fast & json oriented. We would like to better integrate Launch Darkly's output into the rest of our system output. Problem points:

Describe the solution you'd like

What do you think the solution is? We don't have a strong opinion on how to go forward.

We are happy to do a good bit of lifting here. Creating a wrapper around Pino that emulates Winston was the first idea we had. But we were daunted that the Winston library does a lot. Just the logging section of the docs is walloping big. Concerning just the logger alone, there are: object logging, streaming, child logging, meta-data, info objects. The list of features feels like it goes on & on. Our enthusiasm for emulating all of a Winston Logger became quite low.

We thought perhaps we could look at the source code to understand how Winston is used, & create a targeted wrapper, ignore the other capabilities. But at present, this would have to be a recurring task: anytime we upgrade the LD node sdk, we'd need to go re-read the entire library, looking for changes for which Winston logger features are being used.

If we could get some kind of assurance about what Winston features are being used, the situation would be much better & we could proceed with confidence & hopefully ideally release some open source helpers for this purpose. Either a promise that consumption of the Winston Logger's feature-set is not going to change (LD pinky swears they will never use streaming & child loggers, for example), or, if things do change, a new major semantic version as though the LD api had a major change would give us confidence that proceeding was worthwhile.

Describe alternatives you've considered

Additional context

eli-darkly commented 4 years ago

To answer the easiest part first:

We thought perhaps we could look at the source code to understand how Winston is used, & create a targeted wrapper, ignore the other capabilities. But at present, this would have to be a recurring task: anytime we upgrade the LD node sdk, we'd need to go re-read the entire library, looking for changes for which Winston logger features are being used.

I don't see why that would be necessary, given that the SDK explicitly defines the interface for what logger features it uses. There is no requirement that the implementation is Winston or that it implements the Winston API in general, just that it has those four methods.

One thing that is not spelled out in the TS docs there is that the SDK does use the util.format syntax (which Winston supports) for placeholders in debug log messages, so if you wanted to implement string interpolation, you would use util.format. However, that is only done in debug messages (since we want to avoid the overhead of string concatenation for messages that will usually be suppressed) and if we were to change that behavior, it would be in a higher major version since it could be a breaking change for anyone who currently has their own logging implementation that doesn't support placeholders.

As long as we're on version 5.x, you can rely on the fact that the only log methods used by the SDK are those in the LDLogger interface, and that (except in debug messages as mentioned above) the parameters have no special meaning.

eli-darkly commented 4 years ago

So, based on all of the above, I think the simplest approach within the current SDK architecture is just to write a small Pino adapter that has error, warn, info, and debug methods. The first three can assume that their parameters are either a single string or a series of stringifiable values (in actual practice the SDK never passes more than a single value, but given that that's how we defined it in the TS interface, that is the simplest implementation that would make sense). The debug method can do the same or, if you want to do string interpolation to integrate any variable parameters into the message rather than concatenating them in order, call util.format to compute the full message. If it's feasible for you to use such an adapter to channel the output into Pino, then I'd assume that that obviates your other concerns.

eli-darkly commented 3 years ago

Closing this issue due to inactivity. I believe that the significant questions were answered, but if not, please feel free to reopen this.