Access sinks

[bdovaz]

Is your feature request related to a problem? Please describe.

@nblumhardt I am using Serilog.Settings.Configuration and the Serilog.Sinks.Graylog sink.

The problem I have is that it seems that the use case of mixing and reading the appsettings configuration and then configuring or overriding a sink from code is not covered.

Graylog's sink allows for example to set up a transport factory which is obviously something that cannot be set up in a json file: https://github.com/whir1/serilog-sinks-graylog/blob/master/src/Serilog.Sinks.Graylog.Core/GraylogSinkOptions.cs#L211C28-L211C28

Describe the solution you'd like

Although the ideal solution requires more changes in the Graylog repository, this Serilog Core repository should at least expose information that so far is not exposed:

serilog/src/Serilog/LoggerConfiguration.cs

Line 22 in 3016aa3

readonly List<ILogEventSink> _logEventSinks = new();

Proposal:

public class LoggerConfiguration
{
    public IEnumerable<ILogEventSink> Sinks => _logEventSinks;
}

And then:

var loggerConfiguration = new LoggerConfiguration()
        .ReadFrom.Configuration(configuration);

var graylogSink = loggerConfiguration.Sinks.OfType<GraylogSink>().First();

Describe alternatives you've considered

Right now I'm doing it through reflection:

FieldInfo? logEventSinksFieldInfo = loggerConfiguration.GetType().GetField("_logEventSinks", BindingFlags.NonPublic | BindingFlags.Instance);

List<ILogEventSink> logEventSinks = (List<ILogEventSink>)logEventSinksFieldInfo!.GetValue(loggerConfiguration)!;

Additional context
Add any other context or screenshots about the feature request here.

[sungam3r]

Agree. This is a common problem of design with an implicit configuration of sinks from configuration. As I understand Graylog repository may provide needed overload accepting delegate to set Func<ITransport> TransportFactory but it will not help in case of using ReadFrom.Configuration(configuration). I should note that configuration subsystem of Serilog supports some non-trivial features like constructing delegates so with some effort you may achive your goal but I agree that this it is not convenient to do and personally I would not.

Also note that sinks are often considered to be immutable after creating. You provided Func<ITransport> TransportFactory property that exists on options object passed into Graylog sink. GraylogSink may use this options object only in ctor so further modification of it does nothing.

I propose to add special API for one-time sinks configuration. See #1930. @nblumhardt If OK I'm glag to discuss naming and other details.

[bdovaz]

Of course, more changes are needed in the Graylog repository because as you say, once the options is instantiated and the sink constructor is called, it is not possible to change any settings:

https://github.com/serilog-contrib/serilog-sinks-graylog/blob/590183d1b0d04d0d05eaa50c0a3b1d7c2d4daab3/src/Serilog.Sinks.Graylog/LoggerConfigurationGrayLogExtensions.cs#L47

https://github.com/serilog-contrib/serilog-sinks-graylog/blob/590183d1b0d04d0d05eaa50c0a3b1d7c2d4daab3/src/Serilog.Sinks.Graylog/GraylogSink.cs#L19

But I didn't want to create noise on this issue because it is a concrete problem of that Graylog extension implementation on Serilog.

[bartelink]

@bdovaz please don't @ anyone in an issue like this.
This repo has tonnes of watchers and contributors.
If someone is a watcher, they'll get there in their own good time.
This is the only way someone (and the associated contributors, of which there are quite a few for Serilog) can magically do day jobs while also serving this amount of happy customers per 6 weeks: https://www.nuget.org/stats/packages/Serilog?groupby=Version
(I do get that you are also a contributor, and meant no harm. I'm also not suggesting that you're doing this persistently. Just that you can't be too thoughtful when raising issues on a project like this, regardless how of how much you'd value a specific person's perspective. The exception is if you were specifically asked by the person in question to @ them when it's logged in the case of some pre-discussion)

[sungam3r]

it is a concrete problem of that Graylog extension implementation on Serilog.

I understand and exactly because of that I suggested generic API to configure any sink regardless of its implementation capabilities. One-time setup is important thing for me in terms of overall design. I would not expose some list of sinks from configuration object.

[bdovaz]

it is a concrete problem of that Graylog extension implementation on Serilog.

I understand and exactly because of that I suggested generic API to configure any sink regardless of its implementation capabilities. One-time setup is important thing for me in terms of overall design. I would not expose some list of sinks from configuration object.

What I meant (and maybe I was misunderstood) with that sentence is that even if we have exposed your new API design (which I think is great) is that if you look at how the graylog sink consumes the options class, it is too late to change properties of the configuration instance.

In that specific sink more changes are needed and we would have to analyze extension by extension if we have to change the design of each one so that this is possible in all of them. Because there is no common pattern of options + sink.

The ideal pattern of options classes is Microsoft's IOptions<> but I understand that you don't want to have a hard dependency on the Serilog core towards Microsoft extensions.

[nblumhardt]

Hi all! Sorry about the patchy availability here, I'm taking a short break and only jumping online sporadically. Back to full speed late next week.

This sounds like a problem unique to Serilog.Settings.Configuration, to me. Using some kind of static factory method or constructor should enable this without too much additional machinery. It's slightly awkward, but it's also a fairly niche scenario.

The other solution, which I personally prefer, is to just configure the sink in code, and pull the dynamically-varied configuration from your own custom configuration section (probably just a couple of flat values that identify the Graylog endpoint). Simple and reliable :-)

[bdovaz]

I wouldn't say it's quite a niche scenario, more like a more advanced scenario for more complex use.

Composing the settings of each sink with a mix of reading the appsettings file and further configuration of properties that are not possible to configure from a json (because they are not primitive types deserializable from json like a Func<>, Action<>, etc) or that you want to override under certain more complex conditions other than appsettings.{EnvironmentName}.json which is what the base Microsoft.Extensions.Configuration system supports.

It is true that the problem is in the context of this Serilog.Settings.Configuration package but I really do not know if it is possible to solve it without touching anything of the core of Serilog and that is why I have created the issue in this repository.

One option is the API design shown by @sungam3r.

Another option that would only affect the Serilog configuration package could be something like this (if it is possible to do it without touching anything in the Serilog core package which I don't know either):

Add this property in the following class:

public sealed class ConfigurationReaderOptions
{
    public Action<ILogEventSink>? OnSinkCreated { get; init; }
}

Usage:

var logger = new LoggerConfiguration()
    .ReadFrom.Configuration(configuration, new ConfigurationReaderOptions {
        OnSinkCreated = sink => {
            if (sink is GraylogSink graylogSink) {
                // Change sink configuration if possible
            }
        }
    )
    .CreateLogger();

[sungam3r]

if you look at how the graylog sink consumes the options class, it is too late to change properties of the configuration instance.

I 100% understand that and already told about that particular case. Serilog code may expose suggested new APIs to configure sinks in any way sink supports.

is to just configure the sink in code

Or couse, but the more interesting question is about how to provide additional tuning when starting entirely from ReadFrom.Configuration. In my practice I have always configured such sinks from code.

Pros and in the same time cons of suggested ConfigurationReaderOptions.OnSinkCreated design is that it works only for sinks constructed from that configuration API. But what is more important is that you still have no access to created sink. You can not write such API because configuration project does not create sinks explicitly, it calls extension methods which add sinks, see ApplySinks method from configuration repo.

[nblumhardt]

I'm happy leaving this discussion open if it might flush out some workable solution, but I feel like direct access to sinks goes against the grain of the Serilog design, which deliberately hides them so that sinks can be implemented using techniques like wrapping, which would get in the way of this kind of usage.

Take for example the way Serilog.Sinks.Async, Serilog.Sinks.PeriodicBatching, or Serilog.Sinks.Map work - in those cases the actual type of the sink added to the logging pipeline isn't the one you might expect, and there's no single/sensible way to get at an "inner sink" in those cases, since the inner thing might not even be a sink itself (the periodic batching case) or might be created deep inside a separate piece of infrastructure (the map case).

For these reasons I think it's a ship that has sailed. There are definitely other ways Serilog could approach this, with various pros and cons, but because a design has been chosen I think a solution would need to embrace the decisions that have already been made.

Since Serilog.Settings.Configuration is calling those sink creation extension methods, perhaps instead of looking to access the raw sink, an extension point could be created there for resolving parameters to the extension method?

E.g. OnCollectSinkArgs(string sinkName, IDictionary<string, object?> args)? (Likely to itself require some design of course :-))

Hope this helps,
Nick

[sungam3r]

Just for information - I updated proposal in #1930 with the latest changes from dev.