sematext / winston-logsene

Winston Transport for Logsene
http://sematext.com/logsene
Apache License 2.0
13 stars 6 forks source link

Winston-logsene is modifying objects #9

Closed libor-vilimek closed 8 years ago

libor-vilimek commented 8 years ago

We switched from loggly to logsene, because of some memory issues of loggly with big ammount of data and logsene is working fine.

But winston-logsene is modifying our objects that we put into logger. I think this is big issue, the logger should never ever do anything with code, or if it is because of speed (you do not have to create copy of the whole object), it should be possible to turn it on/off in options.

Right now, the winston-logsene is adding "source" field to our object, when we are logging it. For example this code :

var myObj = {hi: 'I am simple object'};
console.log(myObj);
winston.info(myObj);
console.log(myObj);

is having this output

{ hi: 'I am simple object' }
info:  hi=I am simple object
{ hi: 'I am simple object',
  source: 'c:\\prace\\nodejs\\node-template\\node_modules\\winston-logsene\\lib\\index.js' }
megastef commented 8 years ago

Well, why dont you pass a new object with the relevant fields to winston? Putting just your current js object could also trouble with elasticsearch mappings. Imagine an 'id' field, one time as string next as integer - indexing would fail. Anyhow, we could look into an option to clone objects (bad for performance) or check where objects are modified to switch it off.

libor-vilimek commented 8 years ago

Well I personally do it everywhere, therefore my code cause no troubles, but we have a thousands of lines of code from past which we did not write. And after changing logging service, our application stars falling, which I personally think should not happen (services we used before do not change the object we are using).

Having option for just "not send any extra field" like "source" is good enough for us, we do not need it.

However I am just thinking - do you use "toJSON" method? Then all you need is to append things like "source" to object which is created by it, not before. Or do you have another approach?

megastef commented 8 years ago

Looked into the code. There is a rewrite option in the constructor - a function that can create a new object. Actually the only field, which is added is "source", so we can switch that off with an option "setSource: false". How does it sound to you? For moment I would not make it standard, as other user might like the feature that that the node module is added to the log.

libor-vilimek commented 8 years ago

That sounds great, I agree with "not stantard yet" beacuse it can cause troubles for users which is already using it. However if I would be you, I would think about "deprecate" that in future and after some time switch it, because IMHO logger should not cause any potential modifications by default.

megastef commented 8 years ago

Just published a new version 1.1.10, setSource: false is default and the log event should not be modified (but could using the rewrite function in constructor ...) The new option is in the Readme, so if somebody likes the old functionality, it can be added to the constructor.