serilog-contrib / serilog-sinks-slack

A simple (yet customizable) Slack logging sink for Serilog
MIT License
41 stars 27 forks source link

Using property CustomSlackChannel from logEvent as channel name override #44

Closed seirysjs closed 1 year ago

seirysjs commented 1 year ago

If such property exists in logEvent properties, then use it as slack channel name instead. This way each logEvent message can have it's own channel specified.

NerijusD commented 1 year ago

@mgibas Hello, that is needed feature. Who could review and merge this?

seirysjs commented 1 year ago

This also allows scoped Logs to override channel name for example:

using (_logger.BeginScope("{CustomSlackChannel}", customSlackChannel))
{
    _logger.LogCritical(exceptionToLog, message, args);
}
seirysjs commented 1 year ago

@bartelink Hello, please review this, it's highly required feature

TrapperHell commented 1 year ago

Hi @seirysjs, thank you for your contribution.

As bartelink pointed out, please allow a reasonable amount of time before pinging to get something done. We all have jobs and work to do.

While using this library myself I have had a use-case to get the logger to write to different channels - and I got it done without resorting to this.

I will consider your approach and try to balance the easiness which it adds versus any potential pitfalls / feature-bloating. I will keep you posted.

NerijusD commented 1 year ago

Hi @seirysjs, thank you for your contribution.

As bartelink pointed out, please allow a reasonable amount of time before pinging to get something done. We all have jobs and work to do.

While using this library myself I have had a use-case to get the logger to write to different channels - and I got it done without resorting to this.

I will consider your approach and try to balance the easiness which it adds versus any potential pitfalls / feature-bloating. I will keep you posted.

@TrapperHell Could you share your approach, how did you do it? Did you have predefined channels? In our use case we want to have dynamic channel names on each run for our application.

seirysjs commented 1 year ago

Hi @seirysjs, thank you for your contribution.

As bartelink pointed out, please allow a reasonable amount of time before pinging to get something done. We all have jobs and work to do.

While using this library myself I have had a use-case to get the logger to write to different channels - and I got it done without resorting to this.

I will consider your approach and try to balance the easiness which it adds versus any potential pitfalls / feature-bloating. I will keep you posted.

Thank you for your review and insight. I will do my best to not tag people like this in the future anymore. I'm sorry for the disturbance in regards to this PR.

Please do share your approach to have the logger to write to different channels. I would really appreciate it!

TrapperHell commented 1 year ago

As requested, here is the approach that I have used to cater for my need. However please note that this does not support dynamic, runtime slack channel changes.

Did you have predefined channels? In our use case we want to have dynamic channel names on each run for our application.

Yes, the channels were predefined.

The approach is as follows, and powered by the Serilog Expressions library:

Define a logger per channel that you intend to log to, you can do this in the JSON configuration itself, such as:

{
  "Name": "Logger",
  "Args": {
    "configureLogger": {
      "Filter": [
        {
          "Name": "ByIncludingOnly",
          "Args": {
            "expression": "EventId.Id = 1"
          }
        }
      ],
      "WriteTo": [
        {
          "Name": "Slack",
          "Args": { "Configure Slack sink configuration here..." }
        }
      ]
    }
  }
},
{
  ... More loggers here...
}

and then to log you would simply use:

logger.LogInformation(new EventId(1), "Greetings...");

In reality I used another function to resolve the EventId necessary for which message, since it wasn't really dependent on log severity but business-logic.

I will review your changes during this weekend and let you know. Overall it looks fine, however I'm hoping there's a somewhat better options than using .Replace("\"", string.Empty) , and I might be a little pedantic here, but perhaps call the parameter {ChannelOverride} instead of {CustomSlackChannel}

TrapperHell commented 1 year ago

I have taken a better look at your PR, and I can see how in this specific use-case, it would be beneficial to have a property to determine this, instead of relying on logical expressions. I'm not entirely sure about the general necessity for it, but let's assume that it will come in handy.

The main issue at this point, which seems like it may have been overlooked, is how it plays out with the ShowPropertyAttachments property in the log configuration. If this is enabled, than the channel name is also output in the log which is definitely an unintended consequence.

Kindly consider your PR again with this property in mind, and see how you can integrate it in the current system. Thank you!

seirysjs commented 1 year ago

PR updated with solution to ShowPropertyAttachments exposing properties that are meant to be used as config overrides.

Added overrides for CustomUserName and CustomIcon as well. Renamed the property CustomSlackChannel to CustomChannel. Reusing SlackSinkOptions property names that are meant to be overridden.

To override the config properties, it's now necessary to specify them in PropertyOverrideList that was newly added to SlackSinkOptions.

Updated README.md with usage.

NerijusD commented 1 year ago

@TrapperHell will you be able to give another look at this at some point this week?

TrapperHell commented 1 year ago

Just one last thing @seirysjs,

Kindly increment the version number in Serilog.Sinks.Slack.csproj after which I'll approve and merge your contribution.

Thank you so much for your time.

seirysjs commented 1 year ago

@TrapperHell changed version from 2.2.1 to 2.2.2

seirysjs commented 1 year ago

Just one last thing @seirysjs,

Kindly increment the version number in Serilog.Sinks.Slack.csproj after which I'll approve and merge your contribution.

Thank you so much for your time.

Thank you for taking your time to review and merge this

TrapperHell commented 1 year ago

We need your help with this one @mgibas. The NuGet push failed - seems like the API key has expired or is no longer valid.

NerijusD commented 1 year ago

Hello @TrapperHell, @mgibas,

Any updates on publishing new package?

mgibas commented 1 year ago

@TrapperHell token regenerated - see ya next year!