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

Support for Scopes in Logger #471

Merged
merged 9 commits into from
Jun 19, 2019
Merged

Support for Scopes in Logger #471

merged 9 commits into from
Jun 19, 2019

Conversation

karpikpl
Copy link
Contributor

@karpikpl karpikpl commented May 30, 2019 •
edited
Loading

Issue #, if available: outdated readme

Description of changes: Updated dependencies and added support for scopes

I think this is a super important feature for lambda logger.
Logs without scope are pretty much useless if lambda is getting traffic.

After this change you can do:

                using(defaultLogger.BeginScope(awsRequestId))
                {
                    defaultLogger.LogInformation("Hello");

                    using(defaultLogger.BeginScope("Second {0}", "scope456"))
                    {
                        defaultLogger.LogError("In 2nd scope");
                        defaultLogger.LogInformation("that's enough");
                    }
                }

and all your logs will have awsRequestId in them. It makes it easy to find/filter and read logs in cloudwatch.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@karpikpl karpikpl changed the title Fixing readme file for Logging AspNetCore Support for Scopes in Logger Jun 13, 2019
@karpikpl
Copy link
Contributor Author

I would appreciate if someone help me run tests - they are failing for me on master

normj
normj reviewed Jun 17, 2019
View reviewed changes
Libraries/src/Amazon.Lambda.Logging.AspNetCore/LambdaILogger.cs Outdated
// https://docs.asp.net/en/latest/fundamentals/logging.html#scopes
return new NoOpDisposable();
}
public IDisposable BeginScope<TState>(TState state) => ScopeProvider?.Push(state) ?? NullScope.Instance;
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using NullScope from ASP.NET Core internal namespace we should continue to use the existing NoOpDisposable object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I'll bring back NoOpDisposable

@normj
Copy link
Member

normj commented Jun 17, 2019

Thanks for the PR. I'm taking a look and testing in practice. Looks like a good change.

normj
normj reviewed Jun 17, 2019
View reviewed changes
Libraries/src/Amazon.Lambda.Logging.AspNetCore/LambdaILogger.cs Outdated

var components = new List<string>(4);
if (_options.IncludeLogLevel)
{
components.Add($"[{logLevel}]");
}

GetScopeInformation(components, multiLine: _options.IncludeNewline);
Copy link
Member

Choose a reason for hiding this comment

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

I'm seeing scopes always being included regardless of what IncludeScopes is set to. Seems like we need an if check before calling GetScopeInformation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, I've missed it.
I based my implementation on https://github.com/aspnet/Extensions/blob/642b9263abe16ba1cd038f27c8cb4661763ff5bb/src/Logging/Logging.Console/src/ConsoleLogger.cs#L293 but missed that condition

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 would not log scopes even without the check, since Scopes would be set to NullExternalScopeProvider
I think it's more readable with the check like you suggested

@normj
Copy link
Member

normj commented Jun 19, 2019

PR is merged into the dev branch. I'll update and close the PR once the feature has been merged to master and released.

@normj normj merged commit 56f0cf9 into aws:master Jun 19, 2019
@normj
Copy link
Member

normj commented Jun 20, 2019

Version 2.3.0 has been released with this PR. Thanks again for the PR.

@normj normj mentioned this pull request Jun 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normj normj normj 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.
2 participants
@karpikpl @normj