Several improvements to Elasticsearch.Ext.Logging #97

Mpdreamz · 2020-07-16T12:12:13Z

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.

relates: #95

apmmachine · 2020-07-16T12:22:11Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #97 updated]
Start Time: 2020-07-20T16:31:11.836+0000
Duration: 10 min 32 sec

Test stats 🧪

Test	Results
Failed	0
Passed	52
Skipped	4
Total	56

- 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.

russcam

I've had a look through and left some general comments

russcam · 2020-07-21T06:20:34Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+			{
+				_outputThread.Join(1500); // with timeout in case writer is locked
+			}
+			catch (ThreadStateException) { }


is there an internal logger to which this exception can be written?

I looked into fallback loggers in the Mic.Ext.Logging world but there is no such concept. The best we can do is expose callbacks to allow to user to log manually to fallback locations in their application configuration.

This is a carry over of #85 (introduced there not here) that is rewritten in #99 .

russcam · 2020-07-21T06:28:21Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+				settings = new ConnectionConfiguration(nodes[0]);
+			else
+			{
+				settings = new ConnectionConfiguration(connectionPool);


Looks like _options.ShipTo.CreateConnectionPool(); can return null:

ecs-dotnet/src/Elasticsearch.Extensions.Logging/ShipTo.cs

Lines 61 to 86 in 249b86d

internal IConnectionPool? CreateConnectionPool()

{

switch (ConnectionPoolType)

{

// TODO: Add option to randomize pool

case Logging.ConnectionPoolType.Unknown:

case Logging.ConnectionPoolType.Sniffing:

return new SniffingConnectionPool(NodeUris);

case Logging.ConnectionPoolType.Static:

return new StaticConnectionPool(NodeUris);

case Logging.ConnectionPoolType.Sticky:

return new StickyConnectionPool(NodeUris);

// case ConnectionPoolType.StickySniffing:

case Logging.ConnectionPoolType.Cloud:

if (!string.IsNullOrEmpty(ApiKey))

{

var apiKeyCredentials = new ApiKeyAuthenticationCredentials(ApiKey);

return new CloudConnectionPool(CloudId, apiKeyCredentials);

}

var basicAuthCredentials = new BasicAuthenticationCredentials(Username, Password);

return new CloudConnectionPool(CloudId, basicAuthCredentials);

default:

return null;

}

}

but ConnectionConfiguration does not guard against null:

https://github.com/elastic/elasticsearch-net/blob/f1ffbf260132eb83ea4e7950c1bcf19ef679ee6f/src/Elasticsearch.Net/Configuration/ConnectionConfiguration.cs#L210

Good catch!, this is a carry over from #98, will update the meta ticket #95 to follow up in a separate PR

russcam · 2020-07-21T06:33:02Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+
+			var lowlevelClient = new ElasticLowLevelClient(settings);
+
+			_ = Interlocked.Exchange(ref _lowLevelClient, lowlevelClient);


I'm struggling to see what benefit this provides over assigning to the field directly in this case, as the original value is not used and field assignment is atomic on all .NET platforms AFAIK. Is there a benefit that I'm missing?

There is not, carry over from #85 we can address that in a follow up PR.

russcam · 2020-07-21T06:42:32Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+			try
+			{
+				foreach (var logEvent in _messageQueue.GetConsumingEnumerable()) PostEvent(logEvent);
+			}
+			catch
+			{
+				try
+				{
+					_messageQueue.CompleteAdding();
+				}
+				catch { }
+			}


If I understand this correctly, an exception in posting the event will mark the blocking collection as no longer accepting items, causing any subsequent items to be sent to PostEvent directly in EnqueueMessage. Would we want to distinguish the types of exceptions that should cause this behaviour vs. those that shouldn't e.g. are there exceptions the client could throw where this behaviour is undesirable?

This is a carry over from #85 and is now rewritten to move away from BlockingCollection to Channels in #99

russcam · 2020-07-21T06:43:31Z

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

+			var index = string.Format(_options.Index, indexTime);
+
+			var localClient = _lowLevelClient;
+			var response = localClient.Index<StringResponse>(index,


I had a question about bulk, but see that this is addressed in #99 😄

russcam · 2020-07-21T06:45:55Z

tests/Elasticsearch.Extensions.Logging.IntegrationTests/LoggingTests.cs

+			logger.LogError("an error occured");
+
+			// TODO make sure we can await something here on ElasticsearchDataShipper
+			await Task.Delay(TimeSpan.FromSeconds(5));


Could the logger expose a property/event to ascertain that it has sent or is sending events?

That was my intend but I knew the shipper had to be rewritten so I left the TODO :)

Once #99 lands we can expose more callbacks and stats

Mpdreamz · 2020-07-21T08:00:50Z

Thanks for the review @russcam I think most comments should be addressed in #99

Noted the need for CreateConnectionPool not to return null in the meta ticket so we can address this in a smaller dedicated PR.

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs

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

Mpdreamz requested review from gregkalapos and russcam July 16, 2020 12:13

Mpdreamz added 3 commits July 16, 2020 17:18

update NEST in benchmarking tests

5d73505

Fix missing reference

5f49309

Mpdreamz force-pushed the feature/ext-logging-improvements branch from 7f5d9da to 5f49309 Compare July 16, 2020 15:23

move tests over to use ShipTo

1bfc2b2

Mpdreamz mentioned this pull request Jul 20, 2020

Bulk support for extensions logging integration. #99

Merged

dotnet test on sln file runs in parallel

9f42b33

russcam reviewed Jul 21, 2020

View reviewed changes

Mpdreamz mentioned this pull request Jul 21, 2020

[meta] Elasticsearch.Extensions.Logging GA #95

Closed

14 tasks

Mpdreamz requested a review from russcam July 21, 2020 08:00

gregkalapos approved these changes Aug 3, 2020

View reviewed changes

src/Elasticsearch.Extensions.Logging/ElasticsearchDataShipper.cs Show resolved Hide resolved

Mpdreamz merged commit 032ce01 into master Aug 5, 2020

Mpdreamz deleted the feature/ext-logging-improvements branch August 5, 2020 08:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several improvements to Elasticsearch.Ext.Logging #97

Several improvements to Elasticsearch.Ext.Logging #97

Mpdreamz commented Jul 16, 2020

apmmachine commented Jul 16, 2020 •

edited

Loading

Build stats

Test stats 🧪

russcam left a comment

russcam Jul 21, 2020

Mpdreamz Jul 21, 2020 •

edited

Loading

russcam Jul 21, 2020

Mpdreamz Jul 21, 2020

russcam Jul 21, 2020

Mpdreamz Jul 21, 2020

russcam Jul 21, 2020

Mpdreamz Jul 21, 2020

russcam Jul 21, 2020

russcam Jul 21, 2020

Mpdreamz Jul 21, 2020

Mpdreamz commented Jul 21, 2020

	internal IConnectionPool? CreateConnectionPool()
	{
	switch (ConnectionPoolType)
	{
	// TODO: Add option to randomize pool
	case Logging.ConnectionPoolType.Unknown:
	case Logging.ConnectionPoolType.Sniffing:
	return new SniffingConnectionPool(NodeUris);
	case Logging.ConnectionPoolType.Static:
	return new StaticConnectionPool(NodeUris);
	case Logging.ConnectionPoolType.Sticky:
	return new StickyConnectionPool(NodeUris);
	// case ConnectionPoolType.StickySniffing:
	case Logging.ConnectionPoolType.Cloud:
	if (!string.IsNullOrEmpty(ApiKey))
	{
	var apiKeyCredentials = new ApiKeyAuthenticationCredentials(ApiKey);
	return new CloudConnectionPool(CloudId, apiKeyCredentials);
	}

	var basicAuthCredentials = new BasicAuthenticationCredentials(Username, Password);
	return new CloudConnectionPool(CloudId, basicAuthCredentials);
	default:
	return null;
	}
	}


		var lowlevelClient = new ElasticLowLevelClient(settings);

		_ = Interlocked.Exchange(ref _lowLevelClient, lowlevelClient);

Several improvements to Elasticsearch.Ext.Logging #97

Several improvements to Elasticsearch.Ext.Logging #97

Conversation

Mpdreamz commented Jul 16, 2020

apmmachine commented Jul 16, 2020 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

russcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mpdreamz Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mpdreamz commented Jul 21, 2020

apmmachine commented Jul 16, 2020 •

edited

Loading

Mpdreamz Jul 21, 2020 •

edited

Loading