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

Build message before queuing it #540

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Conversation

phillip-haydon
Copy link
Contributor

This change is required for updates to Serilog provider where Serilog uses the base implementation of SendInBackground but depends on IRaygunUserProvider to get the user info from HttpContextAccessor.HttpContext

When the build message is sent as a delegate to Enqueue it gets executed out of context and the information is lost.

There is a minor overhead in memory building the message up front but this only occurs when there's a huge number of exceptions being thrown.

Copy link
Contributor

@sumitramanga sumitramanga left a comment

Choose a reason for hiding this comment

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

Code LGTM 🌴

@sumitramanga
Copy link
Contributor

How much are we talking in terms of " huge number of exceptions" for the overhead issue?

@phillip-haydon
Copy link
Contributor Author

How much are we talking in terms of " huge number of exceptions" for the overhead issue?

When we tested it, we chucked it in a loop sending 100k exceptions to see what happened with the memory, it grew a little bit more than if it was delegated, not enough to cause a problem, but just didn't want people to think we were having a memory leak.

This change is more important than ~10mb of data showing up in memory profiling though.

@phillip-haydon phillip-haydon merged commit f2650f5 into master Aug 13, 2024
1 check passed
@phillip-haydon phillip-haydon deleted the ph/fix-httpcontext-lost branch August 13, 2024 02:43
@phillip-haydon
Copy link
Contributor Author

Possibly fixes #536

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

Successfully merging this pull request may close these issues.

2 participants