improvement: add support for dash symbol in property/key name

[vmandic]

Adds support for passing dashed key / property names so they appear as expected in SEQ @Properties list.

Ref: #49

[nblumhardt]

Thanks for that, @vmandic!

Just thinking - since - is not a valid identifier char in Seq's query language, it's inconsistent to allow it while disallowing other invalid identifiers. Perhaps we get rid of key sanitization altogether, and just allow any character in identifiers?

Seq supports querying with @Properties['some-identifier'] for all cases anyway.

[vmandic]

Very much agreed, will remove that method then.

[nblumhardt]

Thank you - looks great! I'll loop back to this in the next few days, I'll need to quickly run over history and make sure I'm not overlooking anything important :-)

[vmandic]

Great! 👍

Best regards, Vedran Mandić

On 29.03.2022., at 05:51, Nicholas Blumhardt @.***> wrote:

 Thank you - looks great! I'll loop back to this in the next few days, I'll need to quickly run over history and make sure I'm not overlooking anything important :-)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[nblumhardt]

Just letting you know this is not forgotten - on holiday for a week, will loop back soon! 🏝️

[vmandic]

Np, enjoy the time off! 👋

Best regards, Vedran Mandić

On 01.04.2022., at 10:36, Nicholas Blumhardt @.***> wrote:

 Just letting you know this is not forgotten - on holiday for a week, will loop back soon! 🏝️

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[nblumhardt]

Hi Vedran,

Thanks for your patience with this - the holiday was great, thanks!

Unfortunately, with fresh eyes, I'm less certain that we should make this change. As this target has been around a long time, chances are high that users will have signals, alerts, dashboards etc. that depend on the property names from log4net events being processed in the way that they currently are by the app. Breakage in some of these things (especially alerts) can be hard to spot and cause considerable issues if the change isn't noticed.

We tried putting our heads together here and came up with some possible ways forward, e.g. to log both Some-Identifier and SomeIdentifier when the names would differ, but this probably wouldn't be anyone's desired behavior (events would be larger and harder to read).

In your environment, are you planning to stick with/move forward with log4net, or is it generally in "maintenance mode"?

If log4net is going to remain your chosen logger for structured logging, perhaps the better option might be for the new behavior to be opt-in, e.g. by adding something like <sanitizePropertyNames value="false" />. If it's just in maintenance mode, as it will be for many people, the value of an opt-in change is probably less 🤔 - any thoughts?

[vmandic]

Hi Nicholas,

Thanks once again for taking time to look into this. Its no problem for us at the moment as I have downloaded and recompiled the lib for this specific scenario so it works for us. The issue on the project I use it, is that its a long running legacy project o .NET48 overflowed with log4net in all of the source code. So without a major refactoring / change effort to move over onto Serilog it is just not feasible anyhow at the moment for management. I will try and find the time to see if I can implement a log4net property (defaulting to true for sanitization if not specified) as you suggest as that would be a non-breaking change to the current behavior. Thats a great proposal. Give me some time of few weeks and leave this open please.

[nblumhardt]

Awesome, will do - thanks for digging into it, @vmandic 👍