Support additional log levels

[kendallroth]

The LogDNA Ingestion docs mention support for quite a few normal log levels (including CRITICAL). However, this package does not support all of them (including CRITICAL). This has provided a challenge for our team, as we do not want to use FATAL (since that is misleading). However, there is no way that we can see to map between WinstonJS log levels and the supported LogDNA log levels.

Would it be possible to support logging CRITICAL errors through this package?

[kendallroth]

Would it be a matter of simply updating the LOG_LEVELS constant to include CRITICAL, or is there more required? If this simple, is there any reason to avoid updating the log levels to match the LogDNA ingestion docs? I can put in a PR if this is all that is required; however, I am not sure whether this change is desired. If not, please let me know how we could resolve this!

[kendallroth]

Turns out that the Winston logger LogDNA transport is not actually sending CRITICAL log levels to the LogDNA logger package, but instead replacing with the default LogDNA transport log level! I had to patch the logdna-winston package as well to allow a custom remapping of log levels, as it only supports the default NPM-style log levels (not good).

However, I would still recommend updating to allow CRITICAL as a log level in the constants for this package :shrug:, as it is supported by LogDNA (as are a few other log levels).

[kendallroth]

Necessary patch file (useful as workaround until resolved); the other part to get this working for us was in the logdna-winston repository.

diff --git a/node_modules/@logdna/logger/lib/constants.js b/node_modules/@logdna/logger/lib/constants.js
index edba2b1..eca72d6 100644
--- a/node_modules/@logdna/logger/lib/constants.js
+++ b/node_modules/@logdna/logger/lib/constants.js
@@ -15,7 +15,7 @@ module.exports = {
     '^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])\\.)*'
       + '([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9-]*[A-Za-z0-9])$'
   )
-, LOG_LEVELS: ['TRACE', 'DEBUG', 'INFO', 'WARN', 'ERROR', 'FATAL']
+, LOG_LEVELS: ['TRACE', 'DEBUG', 'INFO', 'WARN', 'ERROR', 'CRITICAL', 'FATAL']
 , LOGDNA_URL: 'https://logs.logdna.com/logs/ingest'
 , MAC_ADDR_CHECK: /^([0-9a-fA-F][0-9a-fA-F]:){5}([0-9a-fA-F][0-9a-fA-F])$/
 , MAX_REQUEST_TIMEOUT: 300000

[darinspivey]

Thanks for bringing this up, @kendallroth. We can help get this resolved, and allow me to clarify a few things. What the online docs are trying to explain is that logs come from a variety of sources, and these are some of the common log levels that are seen/used (not necessarily the only ones we support). Clients like @logdna/logger is a different point-of-entry than some of our other tools like the LogDNA Agent, so the rules can differ with the acceptable log levels. We can fix that. Once we do that, the docs should make more sense.

A little bit of history--at one point in time, the log levels were like the Wild West and it was very hard to standardize things like validation rules to keep the data clean. After all, proper log levels will be a finite set of values. In an attempt to put control around that, the logger was restricted to only those log levels that you see now. Other packages like logdna-winston added a map to keep it in line. That's been about a year ago, so we can relax a little bit on the restrictions.

I think it's safe to allow a user-defined set of log levels (and not just add CRITICAL.) Thank you for your diff, but this project boasts 100% test coverage, so any changes will need tests fixed as well. We're happy to take this on. Here's what I'll propose for a solution (and also applicable to the Winston side):

• The logger will accept a custom array of "log_levels". All values will be upper-cased.
• If custom levels are provided, it will completely override the defaults. If some of those same levels are desired, they must be specified in the custom list at instantiation time.
• Validation will still be applied to the "acceptable" list of log levels, whether they're user defined or not
• log level values should not have crazy characters, and will be restricted to A-Z
• Convenience methods such as logger.info will not be created for custom levels. The caller must call logger.log('my log line', MYCUSTOMLEVEL)

If you think this change will solve your issue, we can get it done, provided that this approach is vetted on our side. Thanks.

[kendallroth]

Thanks for the response @darinspivey! The explanation of the docs makes sense now, and your suggestion of allowing user-defined log levels seems like a great solution (and would definitely resolve our issue). On a side note, does this mean that LogDNA can accept a larger variety of log levels than just the common ones listed

Regarding the mention of upper-cased custom log levels; would this still perform a case-insensitive match when comparing? I do agree that custom log levels would completely override the defaults (no merging); as long as documentation continues to mention default levels (as it does), this shouldn't be an issue. Continuing to validate the acceptable log levels makes sense, since the custom levels would be included.

The only question I did have was regarding the change to logger.log(). Currently the function takes a statement and an options object; would this behaviour be changed (or would the custom level need to be specified in options)? I should also mention that I don't directly use this logger; rather, I used it as a dependency of logdna-winston (so I'm not sure how this would affect)?

P.S. I should also have clarified above that the patch file was primarily intended for people needing a workaround in the meanwhile have edited to mention that.

Thanks again for the well-thought response!

[darinspivey]

On a side note, does this mean that LogDNA can accept a larger variety of log levels than just the common ones listed

The short answer is 'yes', but it really depends and is a much longer answer than you want to hear. Most logs come from services like http, syslog, etc, which is a different form of parsing than just "user sends us anything." To try and mitigate some of the potential garbage, we require stricter rules around our clients' (log levels) that are directly controlled by users.

Regarding the mention of upper-cased custom log levels; would this still perform a case-insensitive match when comparing?

Yes, absolutely, case-insensitive comparison. The value that will be sent to the server will be uppercase, though, just to keep it consistent.

Currently the function takes a statement and an options object;

The function can take either an object (which then must specify a level property), or a string (which is expected to be a log level and will error if it's not a valid one).

Also, I'd like to amend my last bullet point about the convenience methods. We cannot add convenience methods for custom types simply to avoid prototype pollution, however I think that if any of the custom levels are also part of our defaults, we can allow those convenience methods. For example, if the custom levels include info, then the logger would still support logger.info('my message'), but something like logger.mycustomlevel('my text') would NOT be supported.

[kendallroth]

The short answer is 'yes', but it really depends and is a much longer answer than you want to hear.

Heh, fair point; I appreciate the shortened version regardless! 😄

The value that will be sent to the server will be uppercase, though, just to keep it consistent.

Completely agree (was fairly certain we were on same page)

The function can take either an object (which then must specify a level property), or a string (which is expected to be a log level and will error if it's not a valid one).

Ah, I must have misread the source code regarding that; I see now that it is clearly listed in main documentation 🤦 .

Overall I think this would satisfy both our use case and a broader assortment of use cases; thanks for looking into this!

[darinspivey]

Ok, great. We will address this ASAP after the long US holiday weekend.

[logdnabot]

:tada: This issue has been resolved in version 2.4.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket:

[kendallroth]

Awesome, thanks for taking care of this!

[darinspivey]

No problem, @kendallroth. In the coming days, we'll be bumping the winston logger to include a change that will allow passthrough custom levels to this logger.