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

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

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

MrPsi
Copy link

@MrPsi MrPsi commented Apr 4, 2024

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

…ng overload that accepts EmailSinkOptions and PeriodicBatchingSinkOptions
@MrPsi
Copy link
Author

MrPsi commented Apr 4, 2024

See issue #130

@ericvb
Copy link
Contributor

ericvb commented Jun 14, 2024

@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
Copy link
Member

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

@nblumhardt
Copy link
Member

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
Copy link
Author

MrPsi commented Jun 27, 2024

Hi,

Thanks for looking at this. I did create an issue in the Serilog.Settings.Configuration repository for this, see serilog/serilog-settings-configuration#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
Copy link
Author

MrPsi commented Jul 1, 2024

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?

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

Successfully merging this pull request may close these issues.

None yet

4 participants