Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document logging stuff #465

Closed
originalfoo opened this issue Jul 27, 2019 · 4 comments · Fixed by #474
Closed

Document logging stuff #465

originalfoo opened this issue Jul 27, 2019 · 4 comments · Fixed by #474
Assignees
Labels
docs Documentation in-progress The problem is being solved currently meta Build environment, github environment, etc.
Milestone

Comments

@originalfoo
Copy link
Member

I'll create a markdown doc with notes about best practice for logging seeing as it turned out to be more nuanced than we originally expected.

Some details in this comment: #349 (comment)

@kvakvs Can you comment with some notes on the Format method(s) you added to the Log class? Which situations is it best to use those?

I'll do a draft of the doc and send a PR for review.

@originalfoo originalfoo added docs Documentation meta Build environment, github environment, etc. labels Jul 27, 2019
@originalfoo originalfoo added this to the 11.0 milestone Jul 27, 2019
@originalfoo originalfoo self-assigned this Jul 27, 2019
@kvakvs
Copy link
Collaborator

kvakvs commented Jul 27, 2019

  • Log.Trace, Log.Debug, Log.Info, Log.Warning, Log.Error -- these format the message in place,
    • ✔ Cheap if there is a const string or a very simple format call with a few args.
    • ✔ Cheap if wrapped in an if (booleanValue) { ... }
    • Log.Debug and Log.Trace are optimized away if not in Debug mode
    • ⚠ Expensive if multiple $"string {interpolations}" are used (like breaking into multiple lines)
  • Log.DebugFormat, Log.InfoFormat, ... - these format message later, when logging. Good for very-very long format strings with multiple complex arguments.
    • ✔ As they use format string literal, it can be split multiple lines without perf penalty
    • 💲 The cost incurred: bulding args array (with pointers)
    • Prevents multiple calls to string.Format as opposed to multiline $"string {interpolations}"
    • Log.DebugFormat is optimized away, others are not, so is a good idea to wrap in if (boolValue)
    • ⚠ Expensive if not wrapped in a if () condition
  • Log.DebugIf, Log.WarningIf, ... - these first check a condition, and then call a lambda, which provides a formatted string.
    • ✔ Lambda building is just as cheap as format args building
    • 💲 The cost incurred: each captured variable (pointer) is copied into lambda
    • ✔ Actual string is formatted ONLY if the condition holds true
    • Log.DebugIf is optimized away if not in Debug mode
    • ⚠ Cannot capture out and ref values
  • Log.NotImpl logs an error if something is not implemented and only in debug mode

@dymanoid
Copy link
Contributor

each captured value is copied into lambda

This is not true. A lambda captures a variable, not a value.

@kvakvs
Copy link
Collaborator

kvakvs commented Jul 27, 2019

A pointer.
Does not matter when the log is executed right away and everything is still equal to their expected values, but this might be important when the log is deferred to logging thread and the string value is produced later.

@dymanoid
Copy link
Contributor

A pointer.

Well... Not a pointer. Semantically - a variable. It's better to think this way. For a reference type, this will be a reference. For a value type, this will be a struct copy indeed. (Which might cause unexpected behavior, but it's off-topic for this issue.)

originalfoo added a commit that referenced this issue Aug 1, 2019
Documented most of the logging stuff, except how to flick debug switches.

Fixes #465
@originalfoo originalfoo added the in-progress The problem is being solved currently label Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in-progress The problem is being solved currently meta Build environment, github environment, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants