search

logdna / nodejs

Node.js library for logging to LogDNA
MIT License
74 stars 66 forks source link

Check if opts has saveServiceCopy property - not if it is truthy #77

Closed MartinRoss-IBM closed 5 years ago

MartinRoss-IBM commented 5 years ago

Previous code was only checking if the opts.saveServiceCopy property was truthy:

if (opts.saveServiceCopy) {
    message.saveServiceCopy = opts.saveServiceCopy;
}

When saveServiceCopy was true it would set it on the message object, however, when saveServiceCopy was false the if statement evaluated false and the property was not set on the message object.

This PR changes the check so that it checks if there is a saveServiceCopy property on the opts object, and if there is then it sets it - rather than checking if the value of the property is truthy.

MartinRoss-IBM commented 5 years ago

@weizou19 - I have provided some tests and tested manually.

I sent the following with the current library version: logdnaClient.log(logdnaMessage1, { level: 'INFO', appOverride: 'MR1' }) logdnaClient.log(logdnaMessage2, { level: 'INFO', appOverride: 'MR2', logSourceCRN: 'XXXX', saveServiceCopy: true }) logdnaClient.log(logdnaMessage3, { level: 'INFO', appOverride: 'MR3', logSourceCRN: 'XXXX', saveServiceCopy: false }) logdnaClient.log(logdnaMessage4, { level: 'INFO', appOverride: 'MR3', logSourceCRN: 'XXXX', saveServiceCopy: 'blah' })

This resulted in the following log lines: STS

Oct 23 17:06:18 ibm-app-connect default INFO MR: testing1 
Oct 23 17:06:18 ibm-app-connect default INFO MR: testing2 
Oct 23 17:06:18 ibm-app-connect default INFO MR: testing3 
Oct 23 17:06:18 ibm-app-connect default INFO MR: testing4

SERVICE LOGS

Oct 23 17:06:18 appconnect MR2 INFO MR: testing2 
Oct 23 17:06:18 appconnect MR3 INFO MR: testing3 
Oct 23 17:06:18 appconnect MR3 INFO MR: testing4

As expected message1 did not have a logSourceCRN so only appears on the STS instance, and message2 sets saveServiceCopy to true so should have a copy in the STS and entry appear in service logs. And the issue is shown with message3 and message4 - these should not appear in the SYS but they do.

With the fix provided under this PR I now see: STS

Oct 23 17:07:39 ibm-app-connect default INFO MR: testing1 
Oct 23 17:07:39 ibm-app-connect default INFO MR: testing2

SERVICE LOGS

Oct 23 17:07:39 appconnect MR2 INFO MR: testing2 
Oct 23 17:07:39 appconnect MR3 INFO MR: testing3

^^ This is the behaviour I think I would expect.

vilyapilya commented 5 years ago

Thank you @MartinRoss-IBM. you PR is approved.

vilyapilya commented 5 years ago

Sorry @MartinRoss-IBM but we have decided to get rid of any opinionated code in our libraries. I have to reject your PR.

MartinRoss-IBM commented 5 years ago

@weizou19 - although the saveServiceCopy option is constant over all log entries and can be moved to the constructor, the appOverride and logSourceCRN need to be specified on a per-log basis so I'm not sure how that would fit with providing them through the constructor...

weizou19 commented 5 years ago

@MartinRoss-IBM Thanks for the info. So I think we probably should support both a customized options for constructor and .log function. Does that work for your use cases?

weizou19 commented 5 years ago

@MartinRoss-IBM We will support the options, but don't explicitly say them in the code. We will simply apply the options to the message object

weizou19 commented 5 years ago

@MartinRoss-IBM This is the PR: https://github.com/logdna/nodejs/pull/79

MartinRoss-IBM commented 5 years ago

@weizou19 - https://github.com/logdna/nodejs/pull/79 looks good to me, I was going to send you a PR that did something similar this morning. I have also pulled down the branch it is on and tested manually and gives us exactly what we need. Thank you.

weizou19 commented 5 years ago

@MartinRoss-IBM : I'm glad that helps! I've merged https://github.com/logdna/nodejs/pull/79 and will publish a new MAJOR version for it. Closing this PR now.