datalust / winston-seq

A Winston v3 transport for Seq
Apache License 2.0
13 stars 3 forks source link

Initial/minimal project setup and features #1

Closed nblumhardt closed 2 years ago

nblumhardt commented 3 years ago
Apollon77 commented 3 years ago

As info what "we" (aka ioBroker) as project use from that winston library is also not that much https://github.com/ioBroker/ioBroker.js-controller/blob/master/lib/logger.js#L58-L97 does some converting stuff and config handling is on https://github.com/ioBroker/ioBroker.js-controller/blob/master/lib/logger.js#L291-L312 ... So if a new library supports that at least I'm happy :-)

rj-xy commented 3 years ago

I've made a start: https://github.com/xy1xy/winston-seq

nblumhardt commented 3 years ago

That's awesome @xy1xy - feel free to PR it up in any state of completeness :-)

rj-xy commented 3 years ago

@Apollon77 I've got the basic implementation up in this repo. If you have some time, could you kick the tyres? I'll do the same.

Apollon77 commented 2 years ago

We will give it a try now ... https://github.com/ioBroker/ioBroker.js-controller/pull/1614 ... testing should start soon

nblumhardt commented 2 years ago

@Apollon77 how did it go? 🤞

Apollon77 commented 2 years ago

Still waiting for the tester to respond ;-)

But @nblumhardt could you check the linked PR please if we can do it that weay ... especially the "altering "props" because we have a bit of a special usecase. Should it work that way?

nblumhardt commented 2 years ago

LGTM, @Apollon77 - will be good to hear how it goes :+1:

foxriver76 commented 2 years ago

@nblumhardt Our problem is, that we want to change the loglevel of the seq transport during runtime. If we do this by overwriting the level attribute of the transport it shows no effect and the loglevel which has been passed to the constructor stays active.

Maybe you have an idea?

nblumhardt commented 2 years ago

@foxriver76 ah, I see! I'm not sure this has ever been explicitly considered as a feature, here; SeqTransport just extends TransportStream from winston-transport, so I assume we'll be inheriting its behavior. Might need some digging around to find out where in Winston the level filter is applied, and what we need to do to support changing it dynamically 🤔

foxriver76 commented 2 years ago

Thanks for your fast response.

We are using other transports too, mainly DailyRotateFile and for at least DailyRotate the change of the loglevel on the fly is working. For the other ones I am personally not sure, because they are only enabled if user is explicitly configuring them but no issues are known.

Change happens on our side for every transport and is currently done just like this: https://github.com/ioBroker/ioBroker.js-controller/blob/c68f371638bea7df442ee7de07226e7cfaab8b78/packages/adapter/lib/adapter/adapter.js#L9091

nblumhardt commented 2 years ago

Thanks, Max. As far as I can tell, the Seq transport should inherit this. Has anyone been able to test the updated version and confirm whether the PR has fixed the issue?

foxriver76 commented 2 years ago

Thanks, Max. As far as I can tell, the Seq transport should inherit this. Has anyone been able to test the updated version and confirm whether the PR has fixed the issue?

Do you mean our PR which introduces your module to our users? Yes, feedback is here https://github.com/ioBroker/ioBroker.js-controller/issues/1322#issuecomment-1005228960

All in all this issue unfortunately still exists.

nblumhardt commented 2 years ago

Thanks @foxriver76!

We've added a test to check that level changes are respected by the Seq transport:

https://github.com/datalust/winston-seq/blob/main/test/integration.test.ts#L229

This passes -- for me :-) -- can you spot any differences with how ioBroker is using the transport?

foxriver76 commented 2 years ago

We are checking, seems to be a fault on our side. Thanks for the investigation

foxriver76 commented 2 years ago

fixed on our side. Thanks again.

Apollon77 commented 2 years ago

So yes, we will soon (TM) release our new version of the ioBroker main component with your library included. tests are going fine

nblumhardt commented 2 years ago

Excellent - thanks for closing the loop.

I think since we've "1.0"'d this library, we should close this issue :-)

All the best with your release!

Apollon77 commented 2 years ago

Thank you very much for your support and officially maintaining this lib!