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

Feature/serilog property filter #90

Merged
merged 11 commits into from
Jul 23, 2020

Conversation

ghayes7
Copy link
Contributor

@ghayes7 ghayes7 commented Jun 19, 2020

Need a way to filter embedded Serilog LogEvent properties from being added to serialized / converted ECS Base.Metadata.

For example if I have embedded properties in my Serilog log event's that I used in the EcsTextFormatterConfiguration.MapCustom to enrich the ECS fields, I won't want it to be added to metadata. There's no other way to filter out a log event property, can't even remove it via EcsTextFormatterConfiguration.MapCustom because it's called after GetMetadata in LogEventConverter.ConvertToEcs. Even if the order was rearranged, still could not remove in MapCustom (i.e. LogEvent.RemovePropertyIfPresent) because the LogEvent is passed by reference so any subsequent renderings of a given logEvent, to different targets will not contain the previously removed property.

Also need a way to set the Serilog Tests LogTestBase classes Formatter property.

@apmmachine
Copy link
Contributor

apmmachine commented Jun 19, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #90 updated]

  • Start Time: 2020-07-17T15:27:36.164+0000

  • Duration: 12 min 14 sec

Test stats 🧪

Test Results
Failed 0
Passed 58
Skipped 6
Total 64

@ghayes7 ghayes7 requested a review from russcam June 30, 2020 17:04
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@Mpdreamz
Copy link
Member

Mpdreamz commented Jul 1, 2020

jenkins test this please

@Mpdreamz Mpdreamz added enhancement New feature or request v1.5.3 labels Jul 1, 2020
@ghayes7
Copy link
Contributor Author

ghayes7 commented Jul 10, 2020

@Mpdreamz @russcam Hey guys, anything I need to do to get this merged in and closed?

@russcam
Copy link
Contributor

russcam commented Jul 14, 2020

@ghayes7, I've been out on PTO and looking at this again now

Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of small things, then I think this is good. Thank you @ghayes7 for your effort here!

src/Elastic.CommonSchema.Serilog/LogEventConverter.cs Outdated Show resolved Hide resolved
src/Elastic.CommonSchema.Serilog/LogEventConverter.cs Outdated Show resolved Hide resolved
@ghayes7 ghayes7 requested a review from russcam July 15, 2020 19:42
@ghayes7
Copy link
Contributor Author

ghayes7 commented Jul 20, 2020

@russcam I think I got everything. Though, github's still saying I missed a suggestion, am I blind?

@ghayes7 ghayes7 requested a review from Mpdreamz July 20, 2020 14:10
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for all your work on this, @ghayes7! 👍

@russcam russcam merged commit a285aa9 into elastic:master Jul 23, 2020
@russcam
Copy link
Contributor

russcam commented Jul 23, 2020

Thanks for your contribution @ghayes7

@ghayes7 ghayes7 deleted the feature/serilogPropertyFilter branch July 25, 2020 17:34
russcam added a commit that referenced this pull request Jun 1, 2021
This commit allows filtering of Serilog LogEvent properties from being added to EcsBase.Metadata

Co-authored-by: Russ Cam <russ.cam@forloop.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request v1.5.3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants