Cleanups for our logging helpers #61669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasonmalinowski
commented
Jun 2, 2022
| @@ -116,7 +116,7 @@ protected virtual CompletionItemRules GetCompletionItemRules(ImmutableArray<(ISy | |||
|
|
|||
| if (completionContext.CompletionOptions.TargetTypedCompletionFilter) | |||
| { | |||
| var tick = Environment.TickCount; | |||
| var stopwatch = SharedStopwatch.StartNew(); | |||
There was a problem hiding this comment.
Note Environment.TickCount has a precision of around 15ms or so, so switching to Stopwatch here is also going to give a lot more accurate timing.
jasonmalinowski
commented
Jun 2, 2022
|
|
||
| // Store the request time metrics per LSP method. | ||
| _requestDurationLogAggregator.IncreaseCount(methodName, Convert.ToDecimal(ComputeLogValue(requestDuration.TotalMilliseconds))); | ||
| _requestDurationLogAggregator.IncreaseCount(methodName, (int)ComputeLogValue(requestDuration.TotalMilliseconds)); |
There was a problem hiding this comment.
In a follow-up, going to move the logarithmic value code in so it can be shared elsewhere, but leaving it here for now.
jasonmalinowski
commented
Jun 2, 2022
| @@ -86,9 +92,9 @@ public string GetBucketsAsString() | |||
| return pooledStringBuilder.ToStringAndFree(); | |||
| } | |||
|
|
|||
| private int GetBucket(decimal value) | |||
| private int GetBucket(int value) | |||
There was a problem hiding this comment.
In practice, we were always converting ints to decimals to pass to here, then doing some decimal math only to convert back to an int. This just keeps it simpler.
jasonmalinowski
commented
Jun 8, 2022
Comment on lines
+45
to
+49
| /// <summary> | ||
| /// The keys in this are either a string or a (string, Guid) tuple. See <see cref="SolutionCrawlerLogger.LogIncrementalAnalyzerProcessorStatistics"/> | ||
| /// for what is writing this out. | ||
| /// </summary> | ||
| private CountLogAggregator<object> _logAggregator = new(); |
There was a problem hiding this comment.
Refactoring this to be more strongly typed is out of scope for this PR. Until this PR, it wasn't even clear what the types were, so this is at least an improvement!
jasonmalinowski
commented
Jun 8, 2022
| @@ -24,7 +24,7 @@ internal sealed partial class WorkCoordinator | |||
| private readonly Registration _registration; | |||
| private readonly object _gate = new(); | |||
|
|
|||
| private readonly LogAggregator _logAggregator = new(); | |||
| private readonly CountLogAggregator<WorkspaceChangeKind> _logAggregator = new(); | |||
There was a problem hiding this comment.
Yes, the WorkCoordinator has two different log aggregators! Until this, it was quite confusing what went in which...
davidwengier
reviewed
Jun 8, 2022
davidwengier
approved these changes
Jun 8, 2022
We (mostly) use these to track time, but rather than just letting them directly take a TimeSpan, we were expecting each of the callers to distill it down to milliseconds and the pass it as an integer. This actually clarifies a current surprise that some of our code calls the data points "ticks" but they're actually logging milliseconds, but that wasn't obvious since there were just ints being passed around everywhere. Even for the cases we were passing around milliseconds, what we were often doing is taking a TimeSpan, grabbing it's TotalMilliseconds and then: - casting it to an int to pass around to our logging functions - casting that to a decimal to pass to the log aggregators - doing decimal division to figure out which bucket it goes in - doing a Math.Floor on that result - casting that result back to an int If we just keep everything internally as an int, the division will already round down and we can avoid all the random extra casting.
Makes it clear this is not a base type of the other ones.
jasonmalinowski
force-pushed
the
log-aggregator-cleanups
branch
from
8219b5c
to
2ce3ef9
Compare
ghost
added this to the 
JoeRobich
added a commit
that referenced
this pull request
Jun 24, 2022
Revert "Merge pull request #61669 from jasonmalinowski/log-aggregator…
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While looking at our existing logging code to figure out what I can reuse, I noticed a few bits that could use some cleanup. Changes:
Commit at a time may or may not help; on one hand each change is pretty mechanical, but the same pieces of code may get touched by multiple commits so that might increase the total amount of code to read pretty significantly.