Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Commit

Permalink
Added original exception to FailureCallback action (#449)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Towmeykaw
Towmeykaw authored Apr 4, 2023
1 parent fef527b commit a01baff
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 4 deletions.
2 changes: 1 addition & 1 deletion sample/Serilog.Sinks.Elasticsearch.Sample/Program.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ static void Main(string[] args)
NumberOfShards = 2,
//BufferBaseFilename = "./buffer",
// RegisterTemplateFailure = RegisterTemplateRecovery.FailSink,
FailureCallback = e => Console.WriteLine("Unable to submit event " + e.MessageTemplate),
FailureCallback = (e, ex) => Console.WriteLine("Unable to submit event " + e.MessageTemplate),
EmitEventFailure = EmitEventFailureHandling.WriteToSelfLog |
EmitEventFailureHandling.WriteToFailureSink |
EmitEventFailureHandling.RaiseCallback,
Expand Down
4 changes: 2 additions & 2 deletions src/Serilog.Sinks.Elasticsearch/Sinks/ElasticSearch/ElasticSearchSink.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ private void HandleException(Exception ex, IEnumerable<LogEvent> events)
{
foreach (var e in events)
{
_state.Options.FailureCallback(e);
_state.Options.FailureCallback(e, ex);
}
}
catch (Exception exCallback)
Expand Down Expand Up @@ -233,7 +233,7 @@ private void HandleResponse(IEnumerable<LogEvent> events, DynamicResponse result
// Send to a failure callback
try
{
_state.Options.FailureCallback(e);
_state.Options.FailureCallback(e, null);
}
catch (Exception ex)
{
Expand Down
2 changes: 1 addition & 1 deletion src/Serilog.Sinks.Elasticsearch/Sinks/ElasticSearch/ElasticsearchSinkOptions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public class ElasticsearchSinkOptions
/// A callback which can be used to handle logevents which are not submitted to Elasticsearch
/// like when it is unable to accept the events. This is optional and depends on the EmitEventFailure setting.
/// </summary>
public Action<LogEvent> FailureCallback { get; set; }
public Action<LogEvent, Exception> FailureCallback { get; set; }

/// <summary>
/// The maximum number of events that will be held in-memory while waiting to ship them to
Expand Down

5 comments on commit a01baff

@nenadvicentic
Copy link
Contributor

Choose a reason for hiding this comment

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

@Towmeykaw If you don't mind me asking, why did you add exception to the callback when LogEvent already contains underlying exception at LogEvent.Event?
And, isn't this breaking change for people who use the callback? If we follow SemVer should this be v10.0.0?

@Towmeykaw
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I follow what you mean by it containing the exception. The LogEvent only contains an exception that has been logged. What this does is that it contains an exception like Http errors connecting to Elastic or mapping errors.
As for the breaking change I marked it as such in the PR.

@nenadvicentic
Copy link
Contributor

Choose a reason for hiding this comment

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

@Towmeykaw I get it now. It is not an exception that we try to log, but the exception produced by logging infrastructure failure. Well, I guess it is minor breakage that is relatively obvious how to fix.

I would only suggest to document the change properly in ReadMe file, so that people are aware of it. And, perhaps, to make that sample app example more clear? Something along this lines:

FailureCallback = (e, ex) => {
    var message = "Unable to submit event " + e.MessageTemplate;

    if (ex is not null)
        message += ", because of: " + ex.Message;

    Console.WriteLine(message);
},

But that's only my 2 cents. @mivano probably has better overview what the next version should be (major, minor or fix).

@mivano
Copy link
Contributor

@mivano mivano commented on a01baff Apr 26, 2023

Choose a reason for hiding this comment

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

It is a binary-breaking change, so we should up the major version. Lets merge and do some house keeping to align the versioning.

@nenadvicentic
Copy link
Contributor

Choose a reason for hiding this comment

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

@mivano Is it possible to first release this fix #519 (once it is finalized) as v9.0.1 or is it too complicated now since this one is already merged?

Please sign in to comment.