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

An invalid log "breaks" the sink #73

Open
Augment1n opened this issue Nov 30, 2022 · 6 comments
Open

An invalid log "breaks" the sink #73

Augment1n opened this issue Nov 30, 2022 · 6 comments

Comments

@Augment1n
Copy link

Describe the bug
If there is any kind of error during a LogEvent's logging, then it breaks the sink entirely, because it does not get rid of the source LogEvent.

To Reproduce
Attached a project. You have to insert your own authenticationId and workspaceId.
AzureAnalyticsExample.zip

Expected behavior
Get rid of the LogEvent and maybe log that 1 LogEvent was removed because of the following error: ...

Screenshots
Console
image

Azure
image

Additional context
#49 is a subset of this

@jessetan
Copy link

jessetan commented Oct 2, 2023

Hi @saleem-mirza, do you have an update on this issue? It comes with a nice reproduction path and fixing this bug would really help to make the sinks output more reliable.

@saleem-mirza
Copy link
Owner

Hi @saleem-mirza, do you have an update on this issue? It comes with a nice reproduction path and fixing this bug would really help to make the sinks output more reliable.

@jessetan this sink just serialize dynamically whatever it gets. Limiting it to "n" number of events blindly is not a good idea. A better approach would be limit/sanitize an object before feeding to sink. Please let me know your thoughts.

@jessetan
Copy link

jessetan commented Oct 4, 2023

I'm not familiar with inner workings of Serilog and Serilog sinks (thanks for your hard work!), but from an application perspective, I would expect to be able to send anything to Serilog and it would log equally well to all sinks.
The example shows that the console logging through Serilog keeps working.

@RomanKreisel
Copy link

Hi @saleem-mirza, do you have an update on this issue? It comes with a nice reproduction path and fixing this bug would really help to make the sinks output more reliable.

@jessetan this sink just serialize dynamically whatever it gets. Limiting it to "n" number of events blindly is not a good idea. A better approach would be limit/sanitize an object before feeding to sink. Please let me know your thoughts.

Right now, even if (partial) information from one log entry would be lost, it would still be an improvement.
Because right now, it blocks logging completely - effectively disabling the whole logging for an application, where one incompatible object is given into logging.

Just wondering: what do other sinks do in such cases?

@saleem-mirza
Copy link
Owner

Hi @saleem-mirza, do you have an update on this issue? It comes with a nice reproduction path and fixing this bug would really help to make the sinks output more reliable.

@jessetan this sink just serialize dynamically whatever it gets. Limiting it to "n" number of events blindly is not a good idea. A better approach would be limit/sanitize an object before feeding to sink. Please let me know your thoughts.

Right now, even if (partial) information from one log entry would be lost, it would still be an improvement. Because right now, it blocks logging completely - effectively disabling the whole logging for an application, where one incompatible object is given into logging.

Just wondering: what do other sinks do in such cases?

In my opinion, application should limit what is being logged. It is not a good idea to just dump a large object blindly. If any object is breaking logging/application, it should be investigated for reason.

@saleem-mirza
Copy link
Owner

I have released another variant of this sink recently. The new sink logs it structurally instead of expanding object. However, it is not compatible with existing sink.

https://github.com/saleem-mirza/serilog-sinks-azure-analytics/tree/vnext
https://www.nuget.org/packages/Serilog.Sinks.AzureLogAnalytics/6.6.7

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

No branches or pull requests

4 participants