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

Update logging to use generated logs #2531

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Update logging to use generated logs #2531

merged 3 commits into from
Sep 12, 2024

Conversation

wabalubdub
Copy link
Contributor

This PR uses the generated logs feature to simplify and reduce the log declarations.

closes #2529

Also my first PR so feedback would be lovely.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Sep 9, 2024 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

JamesNK
JamesNK reviewed Sep 9, 2024
View reviewed changes
src/Grpc.AspNetCore.Server/Internal/GrpcServerLog.cs Outdated
{
private static readonly Action<ILogger, Exception?> _unableToDisableMaxRequestBodySize =
LoggerMessage.Define(LogLevel.Debug, new EventId(1, "UnableToDisableMaxRequestBodySizeLimit"), "Unable to disable the max request body size limit.");
[LoggerMessage(Level = LogLevel.Trace, EventId = 27, EventName = "DeadlineStopped", Message = "Request deadline stopped.")]
Copy link
Member

Choose a reason for hiding this comment

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

Could you preserve the previous order? I'd like to have them ordered by the event id. That way new log methods can be added to the end and it's easy to see the previous event id.

Also, keeping that order also makes code reviewing the changes easier.

@JamesNK
Copy link
Member

JamesNK commented Sep 9, 2024

Hi, thanks for the PR.

FYI this is just one file of many with logs. See https://github.com/search?q=repo%3Agrpc%2Fgrpc-dotnet+LoggerMessage.Define&type=code

You're welcome to update more if you'd like or stick with this one. I'll close the issue once all are updated.

@wabalubdub
Copy link
Contributor Author

Hey, of course! I ordered the methods according to Event ID.
I didn't notice there were more, ill work on the rest of them. would you rather i update the rest of the in this PR? I can probably get to all of them tomorrow.

@JamesNK
Copy link
Member

JamesNK commented Sep 9, 2024

One pr is fine

@wabalubdub
Copy link
Contributor Author

Hey I updated the PR to include changes to all the files that have the LoggerMessage.Define syntax.

A couple notes:

  • I opted to keep the interface as identical as possible, therefore when the call for logging included any formatting beyond a call to the default ToString() I kept the original function signature and made a call to a private generated log after the formatting. The LoggerMessage Attribute doesn't seem to have a way to access member variables of objects that are passed to it so I couldn't generate the formatting automatically as well.
  • I did the same thing when there were conditional statements for optimization like logger.isTraceEnabled()
  • Some of the logging classes are nested in larger classes, the generated logs requires the class to be partial so i made the surrounding class partial as well.

@JamesNK
Copy link
Member

JamesNK commented Sep 12, 2024

Thanks! Looks good to me. There are a lot of changes so I haven't excuasively checked every log, but the ones I did look at were correct.

@JamesNK JamesNK merged commit de9a82f into grpc:master Sep 12, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

Update logging to use generated logs
2 participants
@wabalubdub @JamesNK