Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Future of Castle supported Facilities (for v5) #339

Closed
jonorossi opened this issue Sep 17, 2017 · 27 comments
Closed

Future of Castle supported Facilities (for v5) #339

jonorossi opened this issue Sep 17, 2017 · 27 comments
Milestone

Comments

@jonorossi
Copy link
Member

jonorossi commented Sep 17, 2017

NuGet.org downloads

Package Total downloads Total downloads of 4.0.0
Castle.Windsor 2,014,685 30,514
Castle.LoggingFacility 448,595 14,168
Castle.WcfIntegrationFacility 149,424 2,055
Castle.FactorySupportFacility 61,187 433
Castle.EventWiringFacility 6,319 111
Castle.SynchronizeFacility 3,345 72

My thoughts

  • Startable (in Castle.Windsor.dll)
    • Keep
  • TypedFactory (in Castle.Windsor.dll)
    • Keep
  • Castle.Facilities.Logging
    • Move into Castle.Windsor.dll
  • Castle.Facilities.WcfIntegration
    • Keep as separate DLL
  • Castle.Facilities.FactorySupport (replaced by TypedFactory)
  • Castle.Facilities.EventWiring
  • Castle.Facilities.Synchronize
@ghost
Copy link

ghost commented Sep 17, 2017

My thoughts

  • Startable (in Castle.Windsor.dll)
    • Keep, Cool but let's make stop deterministic or update docs. Seriously had issues with this.
  • TypedFactory (in Castle.Windsor.dll)
    • Could only find FactoryMethodActivator which is a Component Activator Extension in ComponentRegistration. Defo keep!
  • Castle.Facilities.Logging
  • Castle.Facilities.WcfIntegration
    • Separate repo? 2000 downloads? Would like to see this on a different issue tracker. It is just noise for Windsor right now. Keep.
  • Castle.Facilities.FactorySupport (replaced by UsingFactoryMethod)
    • Concur! Deprecate!
  • Castle.Facilities.EventWiring
    • Concur! Deprecate!
  • Castle.Facilities.Synchronize
    • Concur! Deprecate!

@ghost
Copy link

ghost commented Sep 19, 2017

Supports my problem of stopping IStartables: #79

@pnquest
Copy link

pnquest commented Sep 20, 2017

I agree on all counts. I really like the event wiring facility, but it would definitely not be difficult to get along without it.

@jnm2
Copy link
Contributor

jnm2 commented Sep 20, 2017

The only facility I have ever used is TypedFactory. I couldn't use Castle.Windsor without it, so I'm happy to see it's so popular here. 😄

@jonorossi
Copy link
Member Author

I agree on all counts. I really like the event wiring facility, but it would definitely not be difficult to get along without it.

@pnquest it would be good to hear what you are using the event wiring facility for, are you configuring it via XML config?

@generik0
Copy link
Contributor

I use Wcf facility and logging facilities.
It would be very good to get the logging into Windsor. It would it still need, e.g. separate Castle.Windsor-NLog and core-NLog? Or would they all be separated?

For me, even though I use it often, yes the Wcf is fine outside...

@jonorossi
Copy link
Member Author

It would be very good to get the logging into Windsor. It would it still need, e.g. separate Castle.Windsor-NLog and core-NLog? Or would they all be separated?

@generik0 I think you've got a grammar mistake, I can't quite understand what you are asking. The Castle.Windsor-<logger> packages are obsolete and haven't been needed/published for quite a while (they were an empty package with just package dependencies), the Castle.Core-<logger> packages still implement ILoggerFactory and ILogger.

@generik0
Copy link
Contributor

Sorry, I ment the Logging factory.
I did not know I did not need the castle.windsor nlog anymore... thx...

@jonorossi
Copy link
Member Author

I did not know I did not need the castle.windsor nlog anymore... thx...

No problem, I've just unlisted them on nuget.org to make this clearer for everyone. They just get in the way and dependency resolution should be handled by your package manager.

@jonnii
Copy link

jonnii commented Nov 13, 2017

Would it be possible to deprecate the logging facility or move it out of core? We used to rely on it heavily, but it's so easy to implement your own and the logging abstractions are pretty anemic especially when compared to something like serilog.

@jonorossi
Copy link
Member Author

Would it be possible to deprecate the logging facility or move it out of core? We used to rely on it heavily, but it's so easy to implement your own and the logging abstractions are pretty anemic especially when compared to something like serilog.

@jonnii how would Windsor and DynamicProxy log to your logger? This is obviously much less of a problem compared to when we had so many Castle projects that could log, I guess it could just be a an event you could hook for warnings.

I'm interested to see how small your facility is for Serilog if you wouldn't mind sharing.

@jonnii
Copy link

jonnii commented Nov 15, 2017

@jonorossi sure, it's in this gist https://gist.github.com/jonnii/5d9340c38d157df837d20ef881511c3d

We chose to use a subresolver, which I think is the same mechanism that the proper logging facility uses. We re-used the name here because it made migrating easier - there's also a bunch of code we've written (and since deprecated as we've moved everything to serilog) that shims serilog and windsor via extension methods. We also contributed the first version of Castle.Core-Serilog but we no longer use that.

@ghost
Copy link

ghost commented Nov 15, 2017

@jonnii sorry to chime in here but it is refreshing to see this, I have questioned the logging abstractions since I joined. I simply don’t see a need for this either. @jonorossi I fully support these comments. Logging is so simple nowadays. Do we need any abstractions around this? I think we can retire this stuff ...

@jonnii
Copy link

jonnii commented Nov 16, 2017

@jonorossi @Fir3pho3nixx so to be clear this isn't as fully featured as the castle LoggingFacility but it's good enough for our usage, if you need to register a default ILogger or make your own ILoggerFactory you can quite easily. To me the biggest challenge here is that to make this you need to be "an advanced user" who knows how facilities work (or maybe you don't) and as I'm I believe in injecting loggers, as are others, it's definitely "getting started" friction for using the container.

My suggestion is to find a blend of what we have now while removing the logging abstractions, you could have a set of packages for common logging frameworks that "just work" out of the box (e.g. pre-packaged facilities) and a built in generic LoggingFacility which is maybe looks like this:

public class GenericLoggingFacility<T> 
{
  public GenericLoggingFacility(T rootLogger, Func<Type, T> createLogger) { ... }
}

Which wraps some/most of the sub resolver stuff.

In terms of windsor instrumentation there is already a ton of stuff built in, performance counters, configuration warnings, etc... that to me it might make sense to unify some of that and to move to a model with well known hooks instead of a generic logger - this is also useful for situations where you want to use semantic logging as then you have types and values and you can figure how best to log that.

@generik0
Copy link
Contributor

Thats Great, but won’t you need e.g. container.UseNLog() extension (or what ever logger you use) then? As was pointed out windsor and core do hav diagnostic information to log?
This would mean the creation of castle.windsor.Nlog Ike we do with the aspnetcore logging factory.

@jonorossi
Copy link
Member Author

My suggestion is to find a blend of what we have now while removing the logging abstractions, you could have a set of packages for common logging frameworks that "just work" out of the box (e.g. pre-packaged facilities) and a built in generic LoggingFacility which is maybe looks like this

@jonnii sounds good.

@jonnii @Fir3pho3nixx I'm definitely in the same camp, especially that we no longer maintain larger more framework like libraries like MonoRail and ActiveRecord which used logging much more heavily. I think the way forward is to get Windsor to the point it no longer needs to depend on the Castle Core abstractions and then we can move down there.

We should move to a fresh GitHub issue, but it sounds like we are building a new logging facility (rather than moving the current one in from its own DLL) that provides hooks either as an abstract base class or something else. Then we need to put together examples of the 3 logging libraries we are currently supporting so we can try to create the nicest API but without being too inflexible.

Does someone want to take the lead by creating an issue with a proposed API for how to hook into a new logging facility?

@ghost
Copy link

ghost commented Nov 24, 2017

@jonorossi - sounds good. I am busy with deprecation so I won’t have time to look at this just yet.

@generik0
Copy link
Contributor

generik0 commented Feb 3, 2018

I see you have taken the NLog, Log4net and Sirilog into sepatate facilites in castle.core.
Thats really great!!!!
Any chance og ILogger and castle.logging to be taken out of Castle.Core and not be available until you add the loging facility?
If i use e.g. Serilog in ASP.Net project i have:

  • MS ILogger
  • Castle ILogger
  • Serilog ILogger
  • My own Loggin abstractions.

There are just to many ILoggers. And do i really need the Castle.Logging present if i am not going to use it?

@jonorossi
Copy link
Member Author

I see you have taken the NLog, Log4net and Sirilog into sepatate facilites in castle.core.

@generik0 There only ever was one Windsor logging facility (and it is currently unchanged), nothing has moved to Castle Core there always was separate logging adapters/sinks, the only recent change was to deprecate some Windsor hardcoded helpers to set up specific logging for known Castle Core logging adapters (i.e. the LoggerImplementation enum and some other methods).

Any chance og ILogger and castle.logging to be taken out of Castle.Core and not be available until you add the loging facility?

This is the Windsor project, changes to Castle Core should be discussed there, however Windsor still makes use of Castle's ILoggerFactory and ILogger even if the logging facility isn't configured, they are interfaces, most of the implementation is in the adapter packages.

If i use e.g. Serilog in ASP.Net project i have:

MS ILogger
Castle ILogger
Serilog ILogger
My own Loggin abstractions.
There are just to many ILoggers. And do i really need the Castle.Logging present if i am not going to use it?

At the moment yes, Castle's ILogger predates all of them. Please reread my last comment which provides some insight into what I'm thinking we do to deprecate Castle's ILogger: #339 (comment).

@generik0
Copy link
Contributor

generik0 commented Feb 4, 2018

Ok. I just first now noticed the Castle.Services.Logging. https://github.com/castleproject/Core/tree/master/src
Thought it was in extension to this discussion.
My bad...

@ghost
Copy link

ghost commented Apr 23, 2018

@jonorossi

How much of this do we want to action before close?

@ghost
Copy link

ghost commented Apr 23, 2018

The only thing I think we can do realistically is the EventWiring and the Synchronise facility.

** Edited ***

Typed factories are next after this!

@jnm2
Copy link
Contributor

jnm2 commented Apr 24, 2018

What's happening with typed factories now?

@jonorossi
Copy link
Member Author

What's happening with typed factories now?

Pretty sure @Fir3pho3nixx is just referring to wanting to do a significant internal rework to fix the many bugs in it, it isn't being deprecated and probably shouldn't have been mentioned in this issue.

@jnm2
Copy link
Contributor

jnm2 commented Apr 24, 2018

Internal rework sounds awesome! No that's okay, sorry! Just curious.

@jonorossi
Copy link
Member Author

I've removed the Event Wiring, Factory Support and Synchronize facilities in #403. This issue is now done.

@jonnii @Fir3pho3nixx I'll leave it up to one of you (or someone else) to log a new issue describing a possible new streamlined logging facility which could replace the existing one.

@ghost
Copy link

ghost commented May 31, 2018

@jonorossi you have given me good steer on this. Thanks. I will pursue this kind of thing once we have and end to end aspnet support in Windsor. I also need to upgrade our builds first. This is defo next on my hit list ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants