Cleanups for our logging helpers #62171
Merged
Conversation
This file contains hidden or 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
ghost
added
the
Member
Author
|
This is a recreation of #61669 that was reverted due to a regression. After some investigation, we believe the regression is just due to a lack of updated training data for ngen, and the correct process is just to merge this and let training catch up. |
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
0c1b6d8 to
69c194d
Compare
Member
Author
|
@davidwengier Were you so excited to sign off on #61669 that you want to do it a second time? Here's your chance! |
Member
"Hey, that guy signed off on the PR that caused a bunch of perf regressions. We should get him to look at the new one too!" |
ryzngard
approved these changes
Jul 14, 2022
Cosifne
approved these changes
Jul 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.