Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

How to use other loggers? #196

Closed
rektide opened this issue Sep 9, 2020 · 3 comments · May be fixed by OrderMyGear/node-client#2
Closed

How to use other loggers? #196

rektide opened this issue Sep 9, 2020 · 3 comments · May be fixed by OrderMyGear/node-client#2

Comments

@rektide
Copy link

rektide commented Sep 9, 2020

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:

  • The lack of JSON throws off some of log-consumer systems
  • We lack vital meta-data such as when errors are happening
    • We could add that meta-data to Winston too but it's not clear to us we can source the same meta-data in both places. Will these two loggers drift if we change our app logger's metadata?
  • When we change log levels in our app, the launch-darkly output log levels don't change
  • We don't use any fancy outputters, it's all to console, but that could be a point of pain for some

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

  • Giving up & having a bad experience
  • Begging for a switch to a smaller logging library (unlikely)

Additional context

@eli-darkly
Copy link
Contributor

eli-darkly commented Sep 9, 2020

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
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants