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

NLog ElasticSearch Target #369

Merged
merged 14 commits into from
May 8, 2024
Merged

NLog ElasticSearch Target #369

merged 14 commits into from
May 8, 2024

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Apr 7, 2024

NLog Target that exports directly to Elastic Cloud or individual Elasticsearch nodes

Docs Preview: https://github.com/snakefoot/ecs-dotnet/tree/main/src/Elastic.NLog.Targets

@snakefoot snakefoot force-pushed the main branch 2 times, most recently from 344e18b to ee17c0f Compare April 9, 2024 18:15

```csharp
var config = new LoggingConfiguration();
var elasticTarget = new ElasticsearchTarget("elastic") { Layout = new EcsLayout(), NodesUri = "http://localhost:9200" };
Copy link
Member

Choose a reason for hiding this comment

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

Specifying the Layout is not necessary here correct? it defaults to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was mostly to show that the ElasticsearchTarget relies on the EcsLayout, and you can configure the EcsLayout if you want to override the default output.

Mpdreamz
Mpdreamz previously approved these changes Apr 10, 2024
Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

LGTM in principle, what a great addition to datashippers!

Could you address the conflict on the sln file?

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 10, 2024

what a great addition to datashippers!

Yes was just missing a "Flush"-method on the shipper, as both NLog and log4net has the ability to "Flush" its targets/appenders. Flush-with-timeout gives a better shutdown experience, as dispose has no timeout, and will lead to mesage-loss when closing/disposing while burning down the house (timeout limited to 1-2 secs).

Could you address the conflict on the sln file?

Conflicts has been resolved

@Mpdreamz
Copy link
Member

run docs-build

@snakefoot
Copy link
Contributor Author

@Mpdreamz Hope I nailed the integration-test this time. Sorry about the retries.

@Mpdreamz
Copy link
Member

run docs-build

@Mpdreamz
Copy link
Member

@Mpdreamz Hope I nailed the integration-test this time. Sorry about the retries.

Don't be! I'll gladly retry until we get there, looking forward to releasing this!

@snakefoot
Copy link
Contributor Author

snakefoot commented Apr 16, 2024

And one more retry, please :)

@Mpdreamz
Copy link
Member

run docs-build

@snakefoot
Copy link
Contributor Author

Finally green build :)

@Mpdreamz
Copy link
Member

Mpdreamz commented May 8, 2024

run docs-build

Copy link
Member

@Mpdreamz Mpdreamz left a comment

Choose a reason for hiding this comment

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

LGTM!

@Mpdreamz Mpdreamz merged commit dd75ad8 into elastic:main May 8, 2024
5 checks passed
@Mpdreamz
Copy link
Member

Mpdreamz commented May 8, 2024

@snakefoot thanks for this new package!

I will be out starting today for 10 days but will release this as soon as I am back, after looking at the other open PR.

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