Skip to content
This repository has been archived by the owner on Oct 17, 2018. It is now read-only.

Add event ids to all publication sites #105

Closed
wants to merge 1 commit into from
Closed

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Nov 13, 2015

Issue: #94
I renamed logging extension methods to be more EF like.

@anpete @muratg @victorhurdugaci @moozzyk

@@ -154,7 +154,7 @@ private BCryptAlgorithmHandle GetSymmetricBlockCipherAlgorithmHandle(ILogger log

if (logger.IsVerboseLevelEnabled())
{
logger.LogVerboseF($"Opening CNG algorithm '{EncryptionAlgorithm}' from provider '{EncryptionAlgorithmProvider}' with chaining mode CBC.");
logger.LogVerbose(DataProtectionEventId.CngCbcAuthenticatedEncryptionOptions, $"Opening CNG algorithm '{EncryptionAlgorithm}' from provider '{EncryptionAlgorithmProvider}' with chaining mode CBC.");
Copy link

Choose a reason for hiding this comment

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

Event ids should typically be unique within a single logging "category". Suggest you create specific ids for Hmac and SymmetricBlockCipher code paths.

@anpete
Copy link

anpete commented Nov 13, 2015

Per comment, ensure unique event ids per logger category. Also, consider taking ILogger<> as ctor dependency parameter, rather than passing logger instance to individual methods.

@victorhurdugaci
Copy link
Contributor

Our team uses a different style for logging. Take a look at these previous PRs that add logging:

{
public enum DataProtectionEventId
{
KeyServices = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

See the PRs that I mentioned for our logging style. It's more clear

@pakrym
Copy link
Contributor Author

pakrym commented Nov 16, 2015

@victorhurdugaci

In MVC we don't log eventids

In StaticFiles we have LoggerExtensions class with logging messages set as delegate fields and eventId just hardcoded in them.

In Hosting we have LoggerExtensions class with log call's and level check in method bodies and event ids in seperate LoggerEventIds class with const fields. And we don't log eventids in some places (https://github.com/aspnet/Hosting/blob/dev/src/Microsoft.AspNet.Server.Testing/Common/RetryHelper.cs)

So, what is our style for logging?

@victorhurdugaci
Copy link
Contributor

MVC has event ids: https://github.com/aspnet/Mvc/blob/da731fc641f56529328d3cb74d6092310a40627e/src/Microsoft.AspNet.Mvc.Core/Logging/HttpStatusCodeResultLoggerExtensions.cs#L17

Use the delegate version when you have arguments to pass. Use the other when you don't. See this bug: aspnet/Logging#272

The pattern is:

  1. Have the messages as extension methods for ILogger
  2. Have (ideally) all event ids in one place
  3. Make sure it's not possible to create the same message with a different id (that's why we hide the message and error level in the method)
  4. Encapsulate the if (log level enabled) { log(..) } logic

@pakrym
Copy link
Contributor Author

pakrym commented Nov 16, 2015

@pakrym
Copy link
Contributor Author

pakrym commented Nov 16, 2015

We changed them later, I see

@victorhurdugaci
Copy link
Contributor

I don't think MVC did a pass through all their messages to add event ids. They should all have ids.

Event ids should be ideally unique. That's why having them in one place makes things easy. Also, they shouldn't change once set, that's why an enum might not be ideal.

@Eilon
Copy link
Member

Eilon commented Nov 17, 2015

Yeah the pattern seen in https://github.com/aspnet/Mvc/blob/da731fc641f56529328d3cb74d6092310a40627e/src/Microsoft.AspNet.Mvc.Core/Logging/HttpStatusCodeResultLoggerExtensions.cs is the correct pattern. We have plenty of work items logged for us to update places where we don't yet use that new pattern. The new pattern follows all the logging best practices, and is also highly performant.

@pakrym pakrym closed this Nov 18, 2015
@natemcmaster natemcmaster deleted the pakrym/log-eventid branch August 25, 2017 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants