DuendeSoftware / Support

Support for Duende Software products
21 stars 0 forks source link

Add support for multiple implementations of IEventSink #1147

Closed krxprn closed 8 months ago

krxprn commented 8 months ago

Which version of Duende IdentityServer are you using? 6

Which version of .NET are you using? 6

Describe the bug I need to be able to register multiple implementations of IEventSink

A clear and concise description of what the bug is.

To Reproduce Currently we're only able to create one huge class for different sinks which is far from ideal

Steps to reproduce the behavior.

Expected behavior

A clear and concise description of what you expected to happen.

Log output/exception with stacktrace I expect to be able to ToServiceBus : IEventSink ToFile : IEventSink ToSerilog : IEventSink

data

Additional context

This was requested a while back and taken into consideration, but ignored after? https://github.com/IdentityServer/IdentityServer4/issues/3771

AndersAbel commented 8 months ago

.NET 8 and IdentityServer v7 has much improved support for Open Telemetry. We think that Open Telemetry in most cases are a better path forward than the events. We are even considering if we should remove the events in a future release in favour of Open Telemetry.

Could you please tell us a bit more about how you use the events? If you've had a chance to look at the Open Telemetry support, is there anything you need that isn't handled by Open Telemetry?

krxprn commented 8 months ago

Hi AndersAbel,

A couple of use cases:

  1. ClientLastAccessedSink : IEventSink
  2. AuditSink : IEventSink

We're currently licensed for IdentiyServer v6 - unsure about future plans to update that to v7; .NET 8 upgrades have not yet commenced but it's good to have in mind, thank you!

AndersAbel commented 8 months ago

Thanks for getting back. If I guess right from the names these are not for technical logging but rather for audit logging?

While we see that Open Telemetry is the path forward for technical logging/instrumentation we are still investigating how audit logging fits into this. Tentatively audit logging will not be a good fit for Open Telemetry but will rather require something else, like the existing eventing model.

Except for the request for allowing multiple sinks, is the event system working well for your requirements?

krxprn commented 8 months ago

Hi Anders,

This is correct, these are not technical logging bits but audit logging ones.

Yes the event system currently suffice our requirements, the events provide enough of meta data for business logic decisions to be made upon. We currently use couple of channels for event logging - SQL updating Identity Server's client table LastAccessed date, which is really important and would be a driver for business decisions. We also use EventHubs for internal auditing.

As initially stated we'd like to have separate implementations for that instead of bloated logic in a single method.

I hope the above helps! Have a good week ahead.

AndersAbel commented 8 months ago

Thank you for your feedback. I've opened an implementation issue about allowing multiple IEventSink registrations: https://github.com/DuendeSoftware/IdentityServer/issues/1531.