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

Added thread for message processing #14

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

Skaptor
Copy link
Contributor

@Skaptor Skaptor commented Aug 27, 2021

I took the embedded state machine approach and made a state machine based thread that waits roughly 25ms or a message counter to reach a certain batch size for reducing the amount of dispatcher calls. I ran the tests and nothing broke 👍. It is noticeable faster than calling the dispatcher method for each message that is being logged. All suggestions are accepted.

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2021

CLA assistant check
All committers have signed the CLA.

@augustoproiete
Copy link
Member

Thanks @Skaptor ! Looks promising. Will take a deeper look in the next few days 👍

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 👍. Added a few comments

@Skaptor
Copy link
Contributor Author

Skaptor commented Sep 8, 2021

Sure I'll make the adjustments. I maybe broke something because I used the vscode web version

@augustoproiete augustoproiete marked this pull request as draft September 14, 2021 21:42
@TonyValenti
Copy link

Hi! What's the status of this? Really hoping for an update.

@Skaptor
Copy link
Contributor Author

Skaptor commented Oct 18, 2021

Changes are ready, we are waiting for @augustoproiete to approve the latest PR.

@augustoproiete
Copy link
Member

Awesome! @TonyValenti are you able to take this one for a spin?

@augustoproiete augustoproiete marked this pull request as ready for review October 18, 2021 20:18
@augustoproiete augustoproiete linked an issue Oct 18, 2021 that may be closed by this pull request
@Skaptor
Copy link
Contributor Author

Skaptor commented Oct 18, 2021

@augustoproiete what are the latest changes you'd like me to make?

@TonyValenti
Copy link

@augustoproiete - can you publish an updated nuget package I can use?

@augustoproiete
Copy link
Member

@Skaptor None at this point. Will take a deeper look at the code in the next few days

@TonyValenti NuGet package is here (package artifact), unzip into a local feed/folder

@Skaptor
Copy link
Contributor Author

Skaptor commented Oct 19, 2021

@augustoproiete OK. agreed.

@Skaptor
Copy link
Contributor Author

Skaptor commented Dec 1, 2021

Hello, is there any new update on this?

@TonyValenti
Copy link

When will this be committed?

@augustoproiete - If you're not able to maintain this project, can you make me an admin? I have a vested interest in this being advanced.

@Skaptor
Copy link
Contributor Author

Skaptor commented Dec 4, 2021

@TonyValenti did you test the artifact nupkg? how did it work?

@augustoproiete
Copy link
Member

@TonyValenti See messages above - This needs manual testing, didn't hear back from you since Oct 18th #14 (comment)

@TonyValenti
Copy link

Thanks! I'll grab the source and do a deep test later today/tomorrow.

@augustoproiete
Copy link
Member

@TonyValenti How did the test go? I imagine merging this will have an impact on #32 so might be good to get this one merged first and release a new preview

@TonyValenti
Copy link

I wasn't able to figure out how to test it without it being merged. Please merge then I'll review and resolve any issues.

My changes on my PR are minimal to existing code and I'd be surprised if there were any issues that took longer than a second to resolve.

@augustoproiete
Copy link
Member

@TonyValenti Download the NuGet package is here (package artifact), unzip into a local feed/folder

@TonyValenti
Copy link

This works great.

Copy link
Member

@augustoproiete augustoproiete left a comment

Choose a reason for hiding this comment

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

LGTM

@augustoproiete augustoproiete merged commit 8bbe83b into serilog-contrib:master Dec 8, 2021
@augustoproiete
Copy link
Member

@Skaptor your changes have been merged, thanks for your contribution 👍

@Skaptor
Copy link
Contributor Author

Skaptor commented Dec 8, 2021

@augustoproiete Happy to help. Thank you for this sink!

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

Successfully merging this pull request may close these issues.

Implement async batching via queue
4 participants