[SERVER-48330] Add logAttr overload for Milliseconds Created: 20/May/20 Updated: 06/Dec/22 Resolved: 30/Jul/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Storage |
| Affects Version/s: | None |
| Fix Version/s: | None |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Daniel Gottlieb (Inactive) | Assignee: | Backlog - Storage Execution Team |
| Resolution: | Won't Do | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Assigned Teams: |
Storage Execution
|
| Participants: |
| Description |
|
The logging of timing should be in milliseconds with the attribute "durationMillis" (though I believe structured logging manages the "Millis" part already). This is a good candidate for having a `logAttr` overload. |
| Comments |
| Comment by Connie Chen [ 30/Jul/20 ] |
|
We failed to get consensus on how this will work, so we are abandoning this effort. |
| Comment by Bruce Lucas (Inactive) [ 01/Jun/20 ] |
|
The methods you mention for surveying the codebase aren't practical for someone who doesn't do day-to-day development. How about instead of an overload for logAttr if we add (say) logCompletionDuration. This avoids duplicate constants, and is then easily searchable, and I believe reduces the likelihood of accidental misuse, because it has a name that suggests what its proper use is. |
| Comment by Henrik Edin [ 01/Jun/20 ] |
|
I don't think text searching in the codebase is a stronger goal than usability / minimize frustration for engineers. There is value in not having to duplicate constants like this all over the code base. But that doesn't mean a helper like this should be misused, it should be used appropriately and when a different attribute name is needed it should be explicitly named instead of using this helper. Using an overload for this actually makes it easy to find all its uses, just with a different method than text searching. Indexing tools over the code should be able to figure it out or just the simple "find all compile errors" when removing the overload locally can be used. It also allow us to remove the special handling of Duration types in the log system and let this helper handle it instead. |
| Comment by Bruce Lucas (Inactive) [ 26/May/20 ] |
|
I have another concern about this: if I want to survey the code to see if the "durationMillis" attr is being used correctly, I can just search for the strings '"duration"' and '"durationMillis"'; with this change it's no longer possible to find uses of those attributes in any straightforward way. Given the very specific meaning of the "durationMillis" attribute I think it would be best if any uses of it were very intentional. |
| Comment by Henrik Edin [ 26/May/20 ] |
|
This could also handle converting durations of different units to milliseconds before logging. logAttrs(Seconds(3)) should be logged as "durationMillis": 3000 |
| Comment by Bruce Lucas (Inactive) [ 20/May/20 ] |
|
My concern is more along the lines that it might convey the impression to a developer that this will just "do the right thing" for any duration type, particularly as it hides the fact that it will generate a "durationMillis" attribute. Of course developers may make such a mistake anyway, but I think it may be less likely if they have to explicitly type the attribute name "duration". |
| Comment by Daniel Gottlieb (Inactive) [ 20/May/20 ] |
|
My understanding is that a programmer has to explicitly type logAttr(value).
I'll certainly loop back if any of those assumptions do not hold. |
| Comment by Bruce Lucas (Inactive) [ 20/May/20 ] |
|
May need to be careful that this doesn't encourage logging unwanted durations as "duration" attrs - that's only appropriate for logging a timespan that represents the completion of an operation. |