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.

Sign up for GitHub

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

Jump to bottom

Add LogEventDropped handler to the NetworkTarget #5101

Merged
merged 3 commits into from
Nov 17, 2022
Merged

Add LogEventDropped handler to the NetworkTarget #5101

merged 3 commits into from
Nov 17, 2022

Conversation

ShadowDancer
Copy link
Contributor

@ShadowDancer ShadowDancer commented Nov 8, 2022 •
edited by snakefoot

Add LogEventDropped handler to the NetworkTarget. Resolves #5084

As proposed in the ticket copied implementation from AsyncTargetWrapper, but had to make a few tweaks due to nature of NetworkTarget (it can discard messages on multiple occasions, due to message size or number of connections).

This MR assumes that NetworkTarget owns and outlives NetworkSender.

Closes #5084

@welcome
Copy link

welcome bot commented Nov 8, 2022

Thanks for opening this pull request!
We will try to review this soon! Please note that pull requests with unit tests are earlier accepted 👼

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 8, 2022
@ShadowDancer ShadowDancer force-pushed the 5084-network-log-dropping branch 4 times, most recently from fc580ef to 352d7d7 Compare November 9, 2022 22:18
@ShadowDancer ShadowDancer marked this pull request as ready for review November 9, 2022 22:43
@POnakS POnakS mentioned this pull request Nov 10, 2022
Closed
@snakefoot
Copy link
Contributor

@ShadowDancer Thank you for the contribution, and finding all the locations where LogEvents are dropped. Excellent work.

Any reason why the dropped-EventArgs is different from AsyncTargetWrapper ? (Not including the LogEvent)

I recognize that it will give better performance, but curious if the inconsistency is good?

@POnakS
Copy link

POnakS commented Nov 15, 2022

@snakefoot QueuedNetworkSender operates on buffers of bytes, it does not have concept of LogEvent (it operates on buffers instead). I've taken simple approach and unified event across all call sites.

I think LogEvent does not matter for this use case, as I want to know if there is backpressure on logshipper - in that case you probably cannot do anything with these logs anyway.

@ShadowDancer
Add LogEventDropped handler to the NetworkTarget (NLog#5084)
0669c0a
@ShadowDancer
Make INetworkSenderFactory return QueuedNetworkSender
61d92bf 
This simplifies NetworkTarget logic as it does not make sense to have network target without buffer.
@ShadowDancer ShadowDancer force-pushed the 5084-network-log-dropping branch 2 times, most recently from a1866b0 to 61d92bf Compare November 16, 2022 00:28
@pull-request-size pull-request-size bot added size/L and removed size/M labels Nov 16, 2022
@sonarcloud
Copy link

sonarcloud bot commented Nov 16, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

93.3% 93.3% Coverage
0.0% 0.0% Duplication

snakefoot
snakefoot approved these changes Nov 17, 2022
View reviewed changes
Copy link
Contributor

@snakefoot snakefoot left a comment •
edited

Choose a reason for hiding this comment

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

I think it looks good. And thank you for the unit-tests. Any last changes before merging?

@POnakS
Copy link

POnakS commented Nov 17, 2022 •
edited

Not really, go ahead :)

@snakefoot snakefoot merged commit b1bf791 into NLog:dev Nov 17, 2022
@welcome
Copy link

welcome bot commented Nov 17, 2022

Hooray your first pull request is merged! Thanks for the contribution! Looking for more? 👼 Please check the up-for-grabs issues - thanks!
thanks!

@snakefoot snakefoot changed the title Add LogEventDropped handler to the NetworkTarget (#5084) Add LogEventDropped handler to the NetworkTarget Nov 17, 2022
@snakefoot snakefoot mentioned this pull request Nov 17, 2022
Merged
@snakefoot snakefoot added this to the 5.1 milestone Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snakefoot snakefoot snakefoot approved these changes

Assignees
No one assigned
Labels
feature network/NLogViewer-target size/L
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

Introduce event handler LogEventDropped for NetworkTarget
3 participants
@ShadowDancer @snakefoot @POnakS