serilog / serilog-sinks-email

A Serilog sink that writes events to SMTP email
Apache License 2.0
70 stars 68 forks source link

Fix to allow to set Subject or Body using JSON configuration when usi… #131

Open MrPsi opened 3 months ago

MrPsi commented 3 months ago

Fix to allow to set Subject or Body using JSON configuration when using overload that accepts EmailSinkOptions and PeriodicBatchingSinkOptions

MrPsi commented 3 months ago

See issue https://github.com/serilog/serilog-sinks-email/issues/130

ericvb commented 3 weeks ago

@MrPsi I see that your PR didn't make it in the sink email release 4.0.0 Was your PR not seen by @nblumhardt ? I've now the same problem, my json Subject and Body lines are not seen by the email sink :-(

nblumhardt commented 3 weeks ago

Thanks for the nudge, I'll loop back to this and take a look ASAP 👍

nblumhardt commented 1 week ago

A quick status update; I've dug into this and I think the best option to make a quick fix here is to work out why the "complex type" subject doesn't work:

            "subject": {
              "type": "Serilog.Formatting.Display.MessageTemplateTextFormatter, Serilog",
              "outputTemplate": "Serilog test"
            },

As I understand it, this should work today with Serilog.Settings.Configuration 8.0.1, but I haven't had any luck either. I spent a while skimming over possible places this could have broken but I think the only way to get to the bottom of it will be to create a test case over there and debug it.

On the plus side, though, if we can track down why complex object creation isn't working, we should be able to get the fix out quickly in a small patch.

Other approaches, like the one in the PR, would need more time thinking through the API. For example, the proposed change would prevent using an ExpressionTemplate to set the subject via JSON (assuming the configuration system's working as expected).

I'll aim to dig into the Serilog.Settings.Configuration side ASAP, but if anyone manages beat me to it, I'd appreciate the help :-)

MrPsi commented 1 week ago

Hi,

Thanks for looking at this. I did create an issue in the Serilog.Settings.Configuration repository for this, see https://github.com/serilog/serilog-settings-configuration/issues/417 .

If I understand the flow in the configuration correctly it will try to find a constructor in the ObjectArgumentValue.ConvertTo method when, for example, it tries to build the EmailSinkOptions. When it does not find any, it will fall back to _section.Get(toType) on line 58. It is only the constructor that supports complex types, hence it will not work.

The solutions I see is that, either EmailSinkOptions needs a constructor, or the configuration needs to check if it can set properties on the class when it can not find a constructor.

But please double check my findings if you find the time, I might have missed something.

MrPsi commented 4 days ago

I updated the PR and changed the type for subject and body in the constructor to ITextFormatter, which is the same type as the properties have. These can then be specified in the json using complex types like this:

"Args": {
  "options": {
    "subject": {
      "type": "Serilog.Formatting.Display.MessageTemplateTextFormatter, Serilog",
      "outputTemplate": "Serilog test {Message}"
    },
    "from": "from@email.com",
    "to": "to@email.com",
    "host": "localhost",
    "body": {
      "type": "Serilog.Formatting.Display.MessageTemplateTextFormatter, Serilog",
      "outputTemplate": "[{Timestamp:HH:mm:ss} {Level}] {SourceContext}{NewLine}{Message}{NewLine}{Exception}{NewLine}"
    }
  },
  "batchingOptions": {
    "batchSizeLimit": 10,
    "BufferingTimeLimit": "00:00:01"
  },
  "restrictedToMinimumLevel": "Information"
}

Hopefully this is more acceptable and will not break the API?