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

FilteringTargetWrapper: Add support for batch writing + PostFilteringTargetWrapper: performance optimizations #3405

Merged
merged 9 commits into from May 20, 2019

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented May 17, 2019

And allow use of WhenRepeated as Filter:

      <target name="default" xsi:type="FilteringWrapper">
        <filter xsi:type="whenRepeated" layout="${message}" timeoutSeconds="30" action="Ignore" />
        <target name="console" xsi:type="Console" />
      </target>

Also made some optimizations to PostFilteringTargetWrapper so it also supports OptimizeBufferReuse = true.

And also "fixed" PostFilteringTargetWrapper so it also works when writing a single LogEvent, without using BufferingWrapper or AsyncWrapper (More intuitive and user-friendly)

@snakefoot snakefoot force-pushed the repeatedfilterwrapper branch 3 times, most recently from c406e16 to 558eb0f Compare May 18, 2019 00:08
@snakefoot snakefoot changed the base branch from dev to master May 18, 2019 00:11
@snakefoot snakefoot changed the base branch from master to dev May 18, 2019 00:11
@snakefoot snakefoot changed the base branch from dev to master May 18, 2019 00:13
@snakefoot snakefoot force-pushed the repeatedfilterwrapper branch 2 times, most recently from 349b1ab to c2e4c90 Compare May 18, 2019 00:42
@codecov-io
Copy link

codecov-io commented May 18, 2019

Codecov Report

Merging #3405 into master will increase coverage by <1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #3405    +/-   ##
=======================================
+ Coverage      80%     80%   +<1%     
=======================================
  Files         356     357     +1     
  Lines       28205   28281    +76     
  Branches     3750    3769    +19     
=======================================
+ Hits        22559   22660   +101     
+ Misses       4540    4535     -5     
+ Partials     1106    1086    -20

@304NotModified 304NotModified added this to the 4.6.4 milestone May 18, 2019
@304NotModified 304NotModified self-requested a review May 18, 2019 02:59
@304NotModified 304NotModified changed the title FilteringTargetWrapper - Add support for batch writing FilteringTargetWrapper: Add support for batch writing + PostFilteringTargetWrapper: performance optimizations May 18, 2019
@304NotModified
Copy link
Member

Thanks!

Is this a Breaking change? (Removal of ConditionExpression Condition)

@304NotModified 304NotModified added the breaking change Breaking API change (different to semantic change) label May 18, 2019
@304NotModified 304NotModified removed this from the 4.6.4 milestone May 18, 2019
@snakefoot
Copy link
Contributor Author

snakefoot commented May 18, 2019

Is this a Breaking change? (Removal of ConditionExpression Condition)

No breaking change. The existing property:

public ConditionExpression Condition { get; set; }

Is now forwarding into the new Filter-property using ConditionBasedFilter as wrapper

@snakefoot snakefoot force-pushed the repeatedfilterwrapper branch 3 times, most recently from 2ed42c9 to 9324129 Compare May 18, 2019 08:03
@snakefoot
Copy link
Contributor Author

@304NotModified Please include for ver. 4.6.4

@304NotModified 304NotModified removed the breaking change Breaking API change (different to semantic change) label May 18, 2019
@304NotModified 304NotModified added this to the 4.6.4 milestone May 18, 2019
@304NotModified
Copy link
Member

It would be nice if we could deduplicate PostFilteringTargetWrapper.ApplyFilter and FilteringTargetWrapper.Write - any idea how?

@snakefoot
Copy link
Contributor Author

It would be nice if we could deduplicate PostFilteringTargetWrapper.ApplyFilter and FilteringTargetWrapper.Write - any idea how?

Made a commit

@304NotModified
Copy link
Member

cool,

what do you think of snakefoot#8 ? Or are lambdas bad for performance?

@snakefoot
Copy link
Contributor Author

snakefoot commented May 19, 2019

Or are lambdas bad for performance?

Yes bad for performance when doing capture. Was also doing it first (With delegate-caching). Until the other target comes with a dynamic-filter-object. Then it became a TState-parameter (Because delegate-caching was not easy).

@304NotModified
Copy link
Member

304NotModified commented May 19, 2019

OK! Added some docs and now this is perfect IMO :)

@snakefoot
Copy link
Contributor Author

OK! Added some docs and now this is perfect IMO :)

Also happy with the result :)

@snakefoot
Copy link
Contributor Author

@304NotModified Guess you need to restart the appveyor-build

@304NotModified
Copy link
Member

restarted 2nd time 👼

@304NotModified 304NotModified merged commit dfd3845 into NLog:master May 20, 2019
@304NotModified
Copy link
Member

Oops the idea was to merge this on to release/4.6.4. Anyway, canceled the build so no problem.

@snakefoot
Copy link
Contributor Author

Updated the documentation: https://github.com/NLog/NLog/wiki/FilteringWrapper-target and added link from https://github.com/NLog/NLog/wiki/WhenRepeated-Filter

@304NotModified
Copy link
Member

304NotModified commented May 20, 2019

Ah this is also a xsd update

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

Successfully merging this pull request may close these issues.

None yet

3 participants