Add logging for dns lookups with timestamps #3631
Conversation
k8s-ci-robot
added
kind/feature
|
Hi @Tanzp01. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
|
cc @bowei the owner of this directory |
dns/dnsperfgo/main.go
Outdated
| log.Printf("DNS lookup of name %q, err - %v\n", name, err) | ||
| } | ||
| if err != nil { | ||
| log.Printf("Failed DNS lookup of name %q, err - %v\n", name, err) |
There was a problem hiding this comment.
This log message is duplicated by structured errorLogger. Maybe structured errorLogger should always be enabled and replace the existing log message? I don't know if this is common for any automation to traverse these logs and changing log message can lead to some failures.
There was a problem hiding this comment.
Thanks, I have enabled the errorLogger by default and remove the old log.
|
/assign @bowei |
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
Tanzp01
force-pushed
the
add_timestamp_for_dnsperfgo
branch
from
d09cd73 to
65b188b
Compare
|
/lgtm /assign @tosi3k |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jprzychodzen, marqc, Tanzp01 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| startTime := time.Now() | ||
| _, err := lookupFunc(name) | ||
| latency := time.Since(startTime) | ||
| if infoLogger != nil && err == nil { |
There was a problem hiding this comment.
why not route the infoLogger to a dummy logger that doesn't output instead of having if statements?
There was a problem hiding this comment.
I also recommend logging for all lookups, irrespective of err == nil or not.
| _, err := lookupFunc(name) | ||
| latency := time.Since(startTime) | ||
| if infoLogger != nil && err == nil { | ||
| infoLogger.Info("DNS lookup successful, latency recorded", |
There was a problem hiding this comment.
suggest:
infoLogger("lookup", "name", name, "latency_ms", latency.Milliseconds(), "err", err)
Latency we want in milliseconds
Record the error.
| log.Printf("Failed DNS lookup of name %q, err - %v\n", name, err) | ||
| errorLogger.Error("Failed DNS lookup", | ||
| "hostname", name, | ||
| "error", err.Error(), |
There was a problem hiding this comment.
Do you need to call Error? Isn't it extraneous?
| if err != nil { | ||
| log.Printf("Failed DNS lookup of name %q, err - %v\n", name, err) | ||
| errorLogger.Error("Failed DNS lookup", | ||
| "hostname", name, |
There was a problem hiding this comment.
make it easier to maintain -- "name", name
Added 2 structured loggings for:
What type of PR is this?
/kind feature
What this PR does / why we need it:
To better analyse the performance of DNS over time
Special notes for your reviewer:
There were existing loggings, but not structured. I added these 2 opt-in structured logging while keeping the previous loggings, so that any existing analysis based on the logging won't be affected