lowlighter / libs

🍱 Collection of carefully crafted TypeScript standalone libraries. Minimal, unbloated, convenient.
https://jsr.io/@libs
MIT License
123 stars 11 forks source link

Logger addEventListener #46

Closed bpevs closed 5 months ago

bpevs commented 5 months ago

Not sure if you think this is in the scope of this module, but one thing that would be very useful for me would be some way to hook-in a listener, so that I can react to logs (I tend to want to log and also send telemetry in similar locations). I wouldn't mind impl myself, but I wonder if this is something that you think would be useful?

Something like one of these:

const options = { blahblahblah }
const log = new Logger({ level: Logger.level.debug, options, tags })

log.addEventListener(({ level, content }) => {
  // do whatever here
})
const options = { blahblahblah }

options.onLog = ({ level, content }) => {
  // do whatever here
}

const log = new Logger({ level: Logger.level.debug, options, tags })
lowlighter commented 5 months ago

This looks interesting actually, I could see it very useful to collect telemetry and diagnostic usage indeed Since I haven't yet encountered the use-case for this yet, would you mind sharing a bit more details about how you'd think it'd work ? Mainly the following points would need a bit more clarification:

API Interface Implementing EventTarget seems good since it keeps consistency with existing conventions, but I wonder if removeEventListener would ever be called, and it'd probably feel weird to expose dispatchEvent.

Also addEventListener usually take the event name in first argument (which could be the loglevel, like addEventListener("warn", () => {})) and while it could seems a good idea to create events by channels, maybe it'd actually be less ergnomic if users want to trigger their listener for "warn and higher" for example. So I don't know wether we should to this or just offer a generic event listener whatever the channel is triggered.

In this case, maybe just a listeners (either a map/set/array) property could be more fitting, and I guess we could also add an options to populate it directly in the constructor too.

Listeners and log level

Would listeners always react independantly from log level ? Consider the following:

const log = new Logger({level:Logger.levels.error})
log.warn("foo") // Would this be still dispatched to listeners despite not logging ?

If we use the latter pattern mentioned in the previous section, then we could trigger it everytime and it's the listener that would check the loglevel themself. Not sure what makes the more sense here

Listeners arguments

What kind of arguments would you expect to be usable within the listener ? I saw you already listed level and content, but if needed it could also be possible to refactor a bit the lib to offer the caller, timestamps, date, etc.

However I guess they'll still be subject to enabled options because I think caller option probably slow down performances (since it needs to examine stack trace). Maybe it could be mitigated with by passing caller as a lazy function though.

Interoperability with Logger.with()

Currently Logger.with creates a "sub-clone" of a logger which inherits all options from parent logger. Would listeners also be inherited ? And if yes, should they be local to said logger (meaning that they don't bubble up) ? It could also be handled with the event.stopPropagation() API, but maybe it makes less sense if EventTarget is not emitted. Or maybe it makes more sense to not propagate in any case...

bpevs commented 5 months ago

Oh wait yikes haha. Thanks for putting some brain cycles into this! tbh now I think I'm having second thoughts after reading:

Would listeners always react independantly from log level ?

It's a great question to ask, but makes me feel like my feature request is actually kind of improper... reason being that logs are code that have zero impact on a user; the kind of code that is least important. While I think this kind of mechanism could be very convenient, I can't help but think that introducing logic that allows actions to be triggered by the logger itself feels wrong in a sense. Like semantically it reeeeeally should probably be a wrapper or higher-order function that calls both the logger and whatever else you're trying to do at the same time.

lowlighter commented 5 months ago

Yeah I agree that it'd be weird for logging to introduce side effects

I think like you said it'd make more sense to just extend the Logger to perform additional actions in case such a design pattern wants to be introduced by a user. Additionally I guess that if it's to send analytics to a remote server, there's a high change that you use fetch which would mix async/sync behaviours which could be a hell to handle with resources sanitization, etc.

However maybe something like this would be a better pattern:

class Analytics {
  #collected
  collect(data:Record<PropertyKey, unknown>) {
    this.#collected.push(data) // Could also add timestamp, caller data, etc.
  } 
  async [Symbol.asyncDispose]() {
    await fetch(...) // Send collected data, could be a file or a remote server I guess
  }
}

await using a8 = new Analytics()
try {
  throw new Error()
}
catch (error) {
 a8.collect(error)
}
// Normally the async disposal will do the "cleanup" here
bpevs commented 5 months ago

Yaya agree. Could be interesting to have another lib that wraps this one, but I'll close this issue for now since it seems out of scope.