search

log4js-node / logFaces-HTTP

LogFaces HTTP appender for log4js-node
Apache License 2.0
6 stars 3 forks source link

Add Hostname Config #15

Closed ZachHaber closed 2 years ago

ZachHaber commented 2 years ago

Closes #14

I had to base this off of #7 because the required "100%" code coverage for pushing wasn't met by the current master branch.

I also had to bump up the version of tap because the older versions can't run in node 16 (LTS).

ZachHaber commented 2 years ago

@peteriman could you take a look at these PRs?

lamweili commented 2 years ago

@ZachHaber I have pinged @nomiddlename to fix some permissions issues I'm facing with this repository.

ZachHaber commented 2 years ago

I do have a few more adjustments I ended up making on my local copy. I can include them in here, or as separate PRs after this one. Just let me know your thoughts on each:

  1. If there's no message normally, the Errors that were pulled out for use in the stack trace aspect should be used for the message, otherwise you end up with just "C" in the log (no idea why logfaces does that).

  2. The filename output uses a relative path from the cwd, as I felt that full paths were mostly just noise before the useful bit (test/index.js for example).

  3. I also have my local copy adjusted to use Jest as a testing framework (along with all dependencies updated) as I was having difficulties getting Tap to work fully locally. Though I acknowledge that the Tests as written currently will not allow for migrating to ESM packages due to the need to sandbox each individual test from eachother with log4js's global configuration, but we won't be able to migrate to ESM that without modifying base log4js to ESM anyway.

  4. And unfortunately due to my work servers and self-signed certificates and node's opinion of incomplete certificate chains (Unable to Verify the First Certificate Error), I ended up needing to add new Agent({rejectUnauthorized: false}) to my local copy (or LogFaces properly sends the full certificate chain from the pem), which means I won't actually be able to use this as a package at work unless we add the ability to customize the agent in the config. My thought there is adding in an httpsAgentOptions config option which would allow for manually setting up a CA or other options as needed, and the fetch implementation also supports Agent, so we can switch to that in the future when it's no longer experimental

lamweili commented 2 years ago

Let me digest and I'll get back to you. In the meantime, you might want to add the hostname configuration in the README.md. :blush:

nomiddlename commented 2 years ago

@ZachHaber I have pinged @nomiddlename to fix some permissions issues I'm facing with this repository.

I've given the log4js-node-devs group the "Maintain" level of access to this repository - not sure why the org-level permissions aren't used though. Let me know if there's anything you can't do @peteriman

lamweili commented 2 years ago

@ZachHaber I have merged all the existing PRs so that you can start work on your other PRs on a clean base.