Bulk support for extensions logging integration. #99

Mpdreamz · 2020-07-20T14:51:54Z

Continuation of #97

Introduces batching of log events before indexing into Elasticsearch. Powered by System.Threading.Channels. It supports sending every N items or every M interval after the first item was added.

This also introduces the ability to have multiple consumers drain the channel concurrently.

A callback now exists when items are being dropped because the channel is full and when a _bulk request has occurred.

We still need to add

_bulk failure handling
_bulk retry handling (exponential backoff)
_bulk rejection callback for LogEvent

Since this PR and #97 are big enough already will tackle the remained when we pull in the flight PR's.

This also includes a modified version of the example project from @sgryphon 's PR: #69

It also includes a high volume worker that can be started with dotnet run -- high.

apmmachine · 2020-07-20T16:55:18Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #99 updated]
Start Time: 2020-08-05T08:57:27.801+0000
Duration: 10 min 45 sec

Test stats 🧪

Test	Results
Failed	0
Passed	62
Skipped	4
Total	66

examples/Elasticsearch.Extensions.Logging.Example/HighVolumeWorkSimulation.cs

russcam

I've gone through and left some comments

examples/Elasticsearch.Extensions.Logging.Example/Program.cs

examples/Elasticsearch.Extensions.Logging.Example/HighVolumeWorkSimulation.cs

russcam · 2020-08-03T07:03:42Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+									if (_options.IndexOffset.HasValue) indexTime = indexTime.ToOffset(_options.IndexOffset.Value);
+
+									var index = string.Format(_options.Index, indexTime);
+									var indexHeader = new { index = new { _index = index } };


would there be some benefit to introducing a type for this?

There would! Was holding off on dedicated types when following this up with retries, backoff and response failure callbacks.

I'm in two minds to spin off ElasticsearchDataShipper as its own package which would benefit from some more typing and control for the end user as well.

I think as these integrations continue to develop, a separate shipper package would probably be a good idea, to share across integrations, and allow users to use the shipper for their own integrations.

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

src/Elasticsearch.Extensions.Logging/ConsumerBuffer.cs

src/Elasticsearch.Extensions.Logging/ElasticsearchLoggerOptions.cs

src/Elasticsearch.Extensions.Logging/ConsumerBuffer.cs

russcam

I left a few more comments

src/Elasticsearch.Extensions.Logging/ElasticsearchLoggerOptions.cs

russcam · 2020-08-03T23:53:28Z

src/Elasticsearch.Extensions.Logging/ElasticsearchLoggerOptions.cs

+		/// If <see cref="MaxInFlightMessages"/> is reached, <see cref="LogEvent"/>'s will fail to be published to the channel. You can be notified of dropped
+		/// events with this callback
+		/// </summary>
+		public Action<LogEvent> PublishRejectionCallback { get; set; } = e => { };


Suggested change

public Action<LogEvent> PublishRejectionCallback { get; set; } = e => { };

public Action<LogEvent> WriteFailedCallback { get; set; } = e => { };

Maybe could use an event for this?

src/Elasticsearch.Extensions.Logging/ElasticsearchLoggerOptions.cs

russcam · 2020-08-03T23:55:36Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+									if (_options.IndexOffset.HasValue) indexTime = indexTime.ToOffset(_options.IndexOffset.Value);
+
+									var index = string.Format(_options.Index, indexTime);
+									var indexHeader = new { index = new { _index = index } };


I think as these integrations continue to develop, a separate shipper package would probably be a good idea, to share across integrations, and allow users to use the shipper for their own integrations.

russcam · 2020-08-04T00:21:03Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+				// DropWrite will make `TryWrite` always return true, which is not what we want.
+				FullMode = BoundedChannelFullMode.Wait
+			});
+			async Task ConsumeMessages() =>


I was thinking along the lines of

private readonly List<Task> _backgroundTasks = new List<Task>();

for (var i = 0; i < maxConsumers; i++) _backgroundTasks.Add(Task.Factory.StartNew(() => Consume(options), TaskCreationOptions.LongRunning));

with Consume taking ElasticsearchLoggerOptions so that changes to options will affect consumers (might be unexpected if changes to options.Throttles don't affect existing consumers).

- Add some basic integration tests to assert the logs make it in the right index - Rename `ElasticsearchDataProcessor` to `ElasticsearchDataShipper` - Move enrichtment of `LogEvent` out of the shipper and into static helper class. - Rely on elasticsearch to generate ids - Move to PostData.StreamHandler to serialize directly to the IO stream.

…rkSimulation.cs

Co-authored-by: Russ Cam <russ.cam@elastic.co>

Mpdreamz · 2020-08-05T09:03:16Z

@russcam I think this is ready for another round. Your comment about throttles and consume task options is a good one but the throttle options are documented as not picking changes. In general I would like to minimizing support live config changes as much as we need to.

I don't want to support scaling consumers and buffer sizes just yet, most likely worth their own PR 😄

Mpdreamz added enhancement New feature or request v1.6.0 v1.5.3 labels Jul 20, 2020

Mpdreamz requested review from russcam and gregkalapos July 20, 2020 14:51

Mpdreamz force-pushed the feature/ext-logging-bulk branch from 2588f3e to aaa9d6f Compare July 20, 2020 16:41

russcam mentioned this pull request Jul 21, 2020

Several improvements to Elasticsearch.Ext.Logging #97

Merged

Mpdreamz commented Jul 21, 2020

View reviewed changes

examples/Elasticsearch.Extensions.Logging.Example/HighVolumeWorkSimulation.cs Outdated Show resolved Hide resolved

russcam reviewed Aug 3, 2020

View reviewed changes

Mpdreamz requested a review from russcam August 3, 2020 14:45

russcam reviewed Aug 4, 2020

View reviewed changes

Base automatically changed from feature/ext-logging-improvements to master August 5, 2020 08:41

Mpdreamz and others added 10 commits August 5, 2020 10:46

Initial channel backed bulk send implementation

37f4f2c

made most toggles configurable and extended the example

b031c95

Update examples/Elasticsearch.Extensions.Logging.Example/HighVolumeWo…

9c424ae

…rkSimulation.cs

Apply suggestions from code review

e04a6f1

Co-authored-by: Russ Cam <russ.cam@elastic.co>

Rename LifeTime => Lifetime

53255bb

remove stray comment

17f11fd

parse for high argument in logging example only once

5728798

rename PubSub to Channel

462199a

remove enabling debug mode on client

d474514

Mpdreamz force-pushed the feature/ext-logging-bulk branch from 735c888 to d474514 Compare August 5, 2020 08:47

Mpdreamz added 2 commits August 5, 2020 10:52

rename DrainThrottles

e260bb0

Add todo to change callbacks to events

da85fc8

Mpdreamz merged commit b4b207b into master Aug 11, 2020

Mpdreamz deleted the feature/ext-logging-bulk branch August 11, 2020 07:50

russcam removed the v1.5.3 label Jun 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bulk support for extensions logging integration. #99

Bulk support for extensions logging integration. #99

Mpdreamz commented Jul 20, 2020

apmmachine commented Jul 20, 2020 •

edited

Loading

Build stats

Test stats 🧪

russcam left a comment

russcam Aug 3, 2020

Mpdreamz Aug 3, 2020

russcam Aug 3, 2020

russcam left a comment

russcam Aug 3, 2020

russcam Aug 3, 2020

russcam Aug 4, 2020

Mpdreamz commented Aug 5, 2020

	public Action<LogEvent> PublishRejectionCallback { get; set; } = e => { };
	public Action<LogEvent> WriteFailedCallback { get; set; } = e => { };

Bulk support for extensions logging integration. #99

Bulk support for extensions logging integration. #99

Conversation

Mpdreamz commented Jul 20, 2020

apmmachine commented Jul 20, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

russcam left a comment

Choose a reason for hiding this comment

russcam Aug 3, 2020

Choose a reason for hiding this comment

Mpdreamz Aug 3, 2020

Choose a reason for hiding this comment

russcam Aug 3, 2020

Choose a reason for hiding this comment

russcam left a comment

Choose a reason for hiding this comment

russcam Aug 3, 2020

Choose a reason for hiding this comment

russcam Aug 3, 2020

Choose a reason for hiding this comment

russcam Aug 4, 2020

Choose a reason for hiding this comment

Mpdreamz commented Aug 5, 2020

apmmachine commented Jul 20, 2020 •

edited

Loading