CSHARP-4279: Logs truncation #891
Conversation
BorisDog
requested review from
DmitryLukyanov and
JamesKovacs
and removed request for
DmitryLukyanov
|
As I start to review this PR I'm finding a number of naming issues that I would have raised earlier if I had been part of earlier PRs. As a general rule class names should not be plural (e.g., Accordingly, I would like to see the following names changed in the new logging code: EventsLogger => EventLogger It would probably make sense to make these changes in a separate PR addressing only the name changes and then rebase this PR on master after pushing those changes to master. |
|
Boris agrees the renamings should be in a separate PR but he wants to do it after this one, so I'm resuming review of this ignoring the naming issues for now. |
| namespace MongoDB.Driver.Core.Configuration | ||
| { | ||
| /// <summary> | ||
| /// Represents the settings for using SSL. |
There was a problem hiding this comment.
/// Represents the settings for logging.
| public ILoggerFactory LoggerFactory { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the maximal document size in chars |
There was a problem hiding this comment.
/// Gets the maximum document size in chars
"maximal" doesn't sound quite right to me.
| /// Initializes a new instance of the <see cref="LoggingSettings"/> class. | ||
| /// </summary> | ||
| /// <param name="loggerFactory">The logging factory.</param> | ||
| /// <param name="maxDocumentSize">The maximal document size in chars.</param> |
There was a problem hiding this comment.
/// <param name="loggerFactory">The logger factory.</param>
/// <param name="maxDocumentSize">The maximum document size in chars.</param>
|
|
||
| return | ||
| LoggerFactory == rhs.LoggerFactory && | ||
| MaxDocumentSize == rhs.MaxDocumentSize; |
There was a problem hiding this comment.
Optionally could be simplified to:
return
rhs != null && // or equivalently: !object.ReferenceEquals(rhs, null)
LoggerFactory == rhs.LoggerFactory &&
MaxDocumentSize == rhs.MaxDocumentSize;
| /// <param name="obj">The <see cref="System.Object" /> to compare with this instance.</param> | ||
| /// <returns> | ||
| /// <c>true</c> if the specified <see cref="System.Object" /> is equal to this instance; otherwise, <c>false</c>. | ||
| /// </returns> |
There was a problem hiding this comment.
Instead of repeating the doc comments from the base class just use:
/// <inheritdoc/>
| /// </summary> | ||
| /// <returns> | ||
| /// A hash code for this instance. | ||
| /// </returns> |
| public void LogAndPublish<TEvent>(TEvent @event) where TEvent : struct, IEvent | ||
| => LogAndPublish(null, @event); | ||
| public void LogAndPublish<TEvent>(TEvent @event, bool skipLog = false) where TEvent : struct, IEvent | ||
| => LogAndPublish(null, @event, skipLog); |
There was a problem hiding this comment.
It's called skipLogging everywhere else
There was a problem hiding this comment.
Not all overloads of LogAndPublish take the new skipLogging parameter.
I assume that's intentional?
There was a problem hiding this comment.
Renamed.
The other overload is not used in places with where skipping logging is required. So for simplicity added only where it is necessary.
Side note: I believe that this parameter will be removed in CSHARP-3823
Part of DRIVERS-1204