pada57 / serilog-sinks-influxdb

Serilog Sinks for InfluxDB
MIT License
10 stars 9 forks source link

Add option to include/exclude all default fields & customise measurement name #36 #38

Closed peitschie closed 1 year ago

peitschie commented 1 year ago

Add option to include/exclude all default fields and option to change measurement name.

E.g.,

        "sinkOptions": {
          "MeasurementName": "syslog",
          "IncludeDefaultFields": true,

When IncludeDefaultFields is false, the message may be omitted too if it's empty or whitespace only.

Addresses #36

tom-englert commented 1 year ago

This extension is called Serilog.Sinks.InfluxDB.Syslog, so why add an option to turn of Syslog format? Isn't this confusing?

peitschie commented 1 year ago

This extension is called Serilog.Sinks.InfluxDB.Syslog, so why add an option to turn of Syslog format? Isn't this confusing?

I can appreciate the seeming conflict of such a request πŸ™‚ ... but the truth is, your plugin is the best maintained influxdb sink in the serilog ecosystem (including tests even, which is awesome!). I had an internal branch making a similar tweak for my own use, and thought to see if it was of any interest to you.

Though I've deliberately kept the configuration lean to avoid confusion as much as possible, if this a use-case or direction direction you're not wanting to take this sink in, this is completely fine.

pada57 commented 1 year ago

Hello

I created originally with syslog format as influx V1 was built around this standard. Main purpose was chronograf.

Now with V2 it is less strict so we could create a second nugget from same code base and have a base class for sink. This would need explanations in docs for users

Would it be ok?

Le dim. 24 sept. 2023, 00:31, Philip Peitsch @.***> a Γ©crit :

This extension is called Serilog.Sinks.InfluxDB.Syslog, so why add an option to turn of Syslog format? Isn't this confusing?

I can appreciate the seeming conflict of such a request πŸ™‚ ... but the truth is, your plugin is the best maintained influxdb sink in the serilog ecosystem (including tests even, which is awesome!). I had an internal branch making a similar tweak for my own use, and thought to see if it was of any interest to you.

Though I've deliberately kept the configuration lean to avoid confusion as much as possible, if this a use-case or direction direction you're not wanting to take this sink in, this is completely fine.

β€” Reply to this email directly, view it on GitHub https://github.com/pada57/serilog-sinks-influxdb/pull/38#issuecomment-1732423565, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACMCUXM6FETBSJZBNTBXT7DX35PN7ANCNFSM6AAAAAA5CYA4IY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

tom-englert commented 1 year ago

I didn't want to strictly vote against this, but just to discuss all the pros and cons first. Looking at the existing InfluxDB sinks on nuget.org, there is already enough confusion, and adding yet another package would not make it much better. Describing this as "defaults to Syslog, but open for anything else" with a few words in the readme could really be the KISS way without generating too much overhead.

pada57 commented 1 year ago

Or yes just documenting and leaving default option using syslog format for existing users

Le dim. 24 sept. 2023, 16:18, tom-englert @.***> a Γ©crit :

I didn't want to strictly vote against this, but just to discuss all the pros and cons first. Looking at the existing InfluxDB sinks on nuget.org, there is already enough confusion, and adding yet another package would not make it much better. Describing this as "defaults to Syslog, but open for anything else" with a few words in the readme could really be the KISS way without generating too much overhead.

β€” Reply to this email directly, view it on GitHub https://github.com/pada57/serilog-sinks-influxdb/pull/38#issuecomment-1732583507, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACMCUXJKXLESQSM56T52JJ3X4A6KNANCNFSM6AAAAAA5CYA4IY . You are receiving this because you commented.Message ID: @.***>

peitschie commented 1 year ago

I'm easy to help with any approach πŸ™‚

It'd be great if the original sink was still active, as in an ideal world, your stability changes and things would merge back into that package. I can appreciate it's confusing to have a .syslog package able to do things other than syslog... not easy for people to discover, and does mix the approaches a bit in code.

How can I help? Is it worth poking the original fork to see if the author is still around?

tom-englert commented 1 year ago

So with a short description in the readme this would be OK for me.

tom-englert commented 1 year ago

@peitschie sorry, lost track about this. What about adding a short note to the readme, so we can merge this?

peitschie commented 1 year ago

Yikes! Time flies... I lost track too, sorry πŸ˜…

I've added a new section to the readme showing how to get the minimal log format, and added a line to the main intro to call out the plugin flexibility.

Thanks for your consideration here. As ever, happy for any corrections/suggestions/improvements πŸ™‚