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.

Sign up for GitHub

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

Jump to bottom

Introduce Console formatter for Microsoft.Extensions.Logging.Console" #439

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Introduce Console formatter for Microsoft.Extensions.Logging.Console" #439

merged 5 commits into from
Sep 18, 2024

Conversation

Mpdreamz
Copy link
Member

  • Introduce Console formatter Microsoft.Extensions.Logging
  • Add README contents

@Mpdreamz Mpdreamz mentioned this pull request Sep 16, 2024
Closed
@Mpdreamz Mpdreamz linked an issue Sep 16, 2024 that may be closed by this pull request
[FEATURE] Elasticsearch.Extensions.Logging Just Log ECS format to Console from .NET #425
Closed
stevejgordon
stevejgordon reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@stevejgordon stevejgordon left a comment

Choose a reason for hiding this comment

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

A few comments to consider, nothing blocking if we want to get this in and follow up.

There's also use of StringBuilder. Given this is high-volume, do we want to consider refactoring to rented array/Span based processing to improve performance?

src/Elastic.Extensions.Logging.Common/LogEventBuilderExtensions.cs
log.Scopes ??= new List<string>();


var scopeValues = (scope as IEnumerable<KeyValuePair<string, object>>)?.ToList();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is fine for now, but maybe we should consider a lower allocation approach with a rented array for these or even stackalloc for low volumes?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should but this is already in place today, only lights up in the PR because we moved it to a new shared library.

Shall we follow up with that in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, let's create an issue to track it as an enhancement. I'd happily pick it up if you want to assign it to me. I love a bit of Span<T> with the morning coffee!

src/Elastic.Extensions.Logging.Console/EcsConsoleFormatterOptions.cs Show resolved Hide resolved
src/Elastic.Extensions.Logging.Console/README.md Outdated Show resolved Hide resolved
src/Elastic.Extensions.Logging.Common/LogEventBuilderExtensions.cs Show resolved Hide resolved
stevejgordon
stevejgordon approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@stevejgordon stevejgordon 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 9ec7e0c into main Sep 18, 2024
7 checks passed
@Mpdreamz Mpdreamz deleted the feature/ecs-extensions-console-logger branch September 18, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevejgordon stevejgordon stevejgordon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Elasticsearch.Extensions.Logging Just Log ECS format to Console from .NET
2 participants
@Mpdreamz @stevejgordon