[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).

  • LOGV2(0, "Message", "otherPrefix"_attr = duration) -> otherPrefixMillis: 20ms
  • LOGV2(0, "Message", logAttr(value)) -> durationMillis: 20ms
  • LOGV2(0, "Message", millisTypeValue) -> compile error

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.

Generated at Thu Feb 08 05:16:49 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.