search

id0Sch / log4js-json-layout

provides a slim and easy to use json-layout for log4js-node
MIT License
17 stars 8 forks source link

feat: Allow setting dynamic properties with functions #6

Closed veetil09 closed 4 years ago

veetil09 commented 4 years ago

Allows the developers to set dynamic properties with the help of functions instead of static values such as setting transaction-id property determined form the request

veetil09 commented 4 years ago

@id0Sch @ironyman @aalexgabi @engeenity @fluxsauce

Could you please review this PR?

Please let me know if you have any questions

fluxsauce commented 4 years ago

I'm not a formal part of this project, I just use it and submitted some PRs. Here's some feedback, though!

I thought about this and it could work, but it's pretty inflexible as it's currently implemented. Can you sequence it so you pass messages into it? If I were to use something like this, I'd be passing something during the transaction that had the context, such as a X-Request-Id that was shared across multiple services and generated by different services, so generating it within the config wouldn't work in that circumstance.

Can you please include some documentation about this feature in the README.md Options section and provide a example after "More options:" called "Dynamic output:"

id0Sch commented 4 years ago

@veetil09, thanks for taking the time to make changes and improve this lib! I highly appreciate it.

I'm not a formal part of this project, I just use it and submitted some PRs. Here's some feedback, though!

I thought about this and it could work, but it's pretty inflexible as it's currently implemented. Can you sequence it so you pass messages into it? If I were to use something like this, I'd be passing something during the transaction that had the context, such as a X-Request-Id that was shared across multiple services and generated by different services, so generating it within the config wouldn't work in that circumstance.

Can you please include some documentation about this feature in the README.md Options section and provide a example after "More options:" called "Dynamic output:"

@fluxsauce, thanks for taking the time to review this change! I second your concerns and would like to see changes to the README.md that detail the usage and an example. as well as changes to the tests to make sure there are no regressions and also demonstrate how this feature is supposed to be used.

@veetil09, once you commit these changes I will review it again - can't wait to merge it!

veetil09 commented 4 years ago

@fluxsauce I am sorry I am not exactly following you. Can you give me an example? As of now, I wanted to allow a dynamic property to be set.

@id0Sch I will update the README with a use case about this.

veetil09 commented 4 years ago

@id0Sch Updated README and added test coverage

veetil09 commented 4 years ago

@id0Sch ^^

id0Sch commented 4 years ago

I’ll take a look at it in the next few days, I’m currently dealing with a small illness

On Wed, Mar 4, 2020 at 5:50 PM Veetil notifications@github.com wrote:

@id0Sch https://github.com/id0Sch ^^

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/id0Sch/log4js-json-layout/pull/6?email_source=notifications&email_token=ABMX2RFGCCDYW3LU6HUGCC3RFZ2ETA5CNFSM4K7ZP23KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENYSQJY#issuecomment-594618407, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMX2RDQKU2GR7WTODO4SYLRFZ2ETANCNFSM4K7ZP23A .

--

Ido Schachter

veetil09 commented 4 years ago

@id0Sch No rush. Please take care and hope you get well soon :)

fluxsauce commented 4 years ago

Can you give me an example?

Sure. Specifically, using https://www.npmjs.com/package/express-request-id . This adds the property id to the request object based on the X-Request-Id header (if supplied) or generated by uuid. I could use dynamic processing of the context of the message to look for a request object and get that header and not log out the entire request object.

Side note, please be patient :-)

veetil09 commented 4 years ago

@fluxsauce That sounds like an enhancement.

What I am trying to add here is something akin to the tokens feature that is exposed in the pattern layout.

id0Sch commented 4 years ago

aight, I'm going to merge it and publish a new version - probably bumping a minor for this. thanks for your contribution, sorry for the delay!

@fluxsauce, thank you as well!