-
Notifications
You must be signed in to change notification settings - Fork 2
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
Optional structured logging to stdout #20
base: master
Are you sure you want to change the base?
Conversation
If the `KENTIK_LOG_FMT` environment variable is set to `json`, then output a more structured log message. Like so: ``` { "time": "2022-08-30T23:16:00.441375151Z", "level": "Info", "prefix": "[CHF] ", "msg": "HTTP SendHttpRequest 1: online for 1013:sflow_demo:1011", "caller": { "file": "/home/alistair/src/chf/pkg/common/tx/http/http.go", "line": 229 } } ``` More examples: ``` {"time":"2022-08-30T23:16:00.430555742Z","level":"Info","prefix":"[CHF] (netclass)","msg":"Added 65486 cloud sets","caller":{"file":"/home/alistair/src/chf/pkg/client/netclass/cloud/cloud.go","line":117}} {"time":"2022-08-30T23:16:00.430599005Z","level":"Info","prefix":"[CHF] ","msg":"Tag client configured with 800ms timeout when fetching tag batches","caller":{"file":"/home/alistair/src/chf/pkg/client/client.go","line":2090}} {"time":"2022-08-30T23:16:00.431913228Z","level":"Info","prefix":"[CHF] (netclass)","msg":"1013 Added 0 application sets","caller":{"file":"/home/alistair/src/chf/pkg/client/netclass/netclass.go","line":134}} {"time":"2022-08-30T23:16:00.431937524Z","level":"Info","prefix":"[CHF] (netclass)","msg":"Starting with no network classification rules, deviceType=router, subtype=advanced_sflow, host-based device? false","caller":{"file":"/home/alistair/src/chf/pkg/client/netclass/netclass.go","line":149}} {"time":"2022-08-30T23:16:00.432362486Z","level":"Info","prefix":"[CHF] ","msg":"SNMP running interface loader","caller":{"file":"/home/alistair/src/chf/pkg/client/interfaces/interfaces.go","line":104}} ```
logger/logger_writer.go
Outdated
Time time.Time `json:"time"` | ||
Level string `json:"level"` | ||
Prefix string `json:"prefix"` | ||
Message string `json:"msg"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we change this to message
to match zerolog?
Message string `json:"msg"` | |
Message string `json:"message"` |
Example from zerolog:
{
"level":"warn",
"requestID":"insights_162c07ce-b6a6-41fd-a08b-1c3dc29db242",
"companyID":3041,
"userID":6643,
"error":"pq: column \"kt_aws_src_region\" does not exist",
"query":"/* service: insights , insightName: comparison.aws.region_to_region.internal */ SELECT *, (f_sum_both_bytes * 8) / (1661863801 - 1661259001) AS bps FROM ( SELECT kt_aws_src_region, kt_aws_dst_region, sum(both_bytes) as f_sum_both_bytes, rank() OVER ( ORDER BY sum(both_bytes) DESC ) AS rank FROM all_devices WHERE i_fast_dataset = TRUE AND app_protocol IN (0, 1, 2, 3, 4) AND i_sub_limit = 300 AND simple_trf_prof = 'internal' AND kt_cloud_provider = $_kntq$AWS$_kntq$ AND ctimestamp > 1661259001 AND ctimestamp <= 1661863801 AND kt_aws_src_region != kt_aws_dst_region AND i_device_type != 'router' GROUP BY kt_aws_src_region, kt_aws_dst_region ORDER BY f_sum_both_bytes DESC LIMIT 100 ) sq1",
"time":"2022-08-30T12:50:02Z",
"caller":"broker.go:391",
"message":"failed to run query"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, sure. See 05c2e69
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two questions re: "prefix"
- Are the reasons for adding a prefix in logs still valid?
- Assuming we can't remove prefix, can we omit the square brackets in the JSON version?
``` { "time": "2022-08-31T13:30:00.954336665Z", "level": "info", "prefix": "[CHF]", "message": "Sending over http to [http://127.0.0.1:20012] -- Max conn Q: 1000", "caller": "/home/alistair/src/chf/pkg/client/packer.go:241" } ```
The prefix is just a string that callers provide, there's been a convention to put brackets around the prefix, and in some cases there are multiple "prefixes" in the string: {
"time": "2022-08-31T13:30:04.461727007Z",
"level": "info",
"prefix": "[CHF] [BGP]",
"message": "Received EOR: &{TimeFloat:0 Neigh:{IP:::1 Msg:{Update:{Attribute:{ASPath:[] ASSet:[] ConfedPath:[] ConfedSet:[] Origin: Community:[] ExtCommunity:[] PrefixSid:{LabelIndex:0}} Announce:{IPs:map[] IP6s:map[] IPL3VPN:map[] IP6L3VPN:map[] IPMPLS:map[] IP6MPLS:map[]} Withdraw:{IPs:[] IP6s:[] IPL3VPN:[] IP6L3VPN:[] IPMPLS:[] IP6MPLS:[]} EOR:{AFI:ipv6 SAFI:unicast}}}} Notification:}",
"caller": "/home/alistair/src/chf/pkg/common/bgp/bgp.go:180"
} In 05c2e69 I at least trimmed the trailing whitespace, but I'm not sure that removing the brackets makes sense. Happy to be convinced otherwise though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Good idea to make caller
match too.
I worry this will prolong golog's existence but the short term benefits are worth it.
In case it needs to be said, there are still reasons to prefer zerolog. Off the top of my head:
- More efficient memory usage
- Context-based logging lets us attach values to the logger that persist through the call stack.
Yeah, I agree completely. For me the biggest benefit of zerolog is that it's actually structured logging so arbitrary K/Vs can be attached to the log rather than crafted into the message sentence. |
…er` goroutine - Moved "string" formatting from `queueMsg` to `(* logMessage).asString` - `(* logMessage).asString` called from `logWriter`, *not* `queueMsg` - teeing the message also moved from `queueMsg` to `logWriter` - Moved call to `(* logMessage).asJSON` from `printStd` to `logWriter` - `logWriter` calls either `(* logMessage).asJSON` or `(* logMessage).asString` - NOTE: JSON formatting is *default* - enable string formatting: set KENTIK_LOG_FMT to non-empty string that is not `json`
) | ||
|
||
type logEntryStructured struct { | ||
Time time.Time `json:"time"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the default string serialization format of time.Time
is not suitable for logging. I'm curious what the output JSON actually looks like here.
You may want to have this struct implement the json.Marshaler interface so you can format the time explicitly.
Looking at the tests, I don't see any that actually verify the JSON output format. If you wish to do that and need an example, I have some tests that do that here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default serialization:
{"time":"2022-09-07T19:32:55.049633946Z","name":"./kproxy [2189:versa_br1_our1:404050] [3044075]","level":"info","prefix":"[CHF]","caller":"/home/debian/src/chf/pkg/common/tx/http/http.go:84","message":"HTTP sendTarget 3: online for https://flow.OUR1.kentik.com/chf, UA: Kentik: dirty-babf062ea69594d98297c1de532b1bcf7939c016 Built on Linux 5.10.16.3-microsoft-standard-WSL2 x86_64 [go version go1.18.5 linux/amd64] Debian GNU/Linux 9.13 (stretch) (Tue, 6 Sep 2022 21:31:54 -1000): 2189:versa_br1_our1:404050"}
{"time":"2022-09-07T19:32:56.603166385Z","name":"./kproxy [2189:versa_br2_our1:404055] [3044066]","level":"info","prefix":"[CHF]","caller":"/home/debian/src/chf/pkg/common/tx/http/http.go:84","message":"HTTP sendTarget 3: online for https://flow.OUR1.kentik.com/chf, UA: Kentik: dirty-babf062ea69594d98297c1de532b1bcf7939c016 Built on Linux 5.10.16.3-microsoft-standard-WSL2 x86_64 [go version go1.18.5 linux/amd64] Debian GNU/Linux 9.13 (stretch) (Tue, 6 Sep 2022 21:31:54 -1000): 2189:versa_br2_our1:404055"}
Good enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah actually I think that should be fine. Thanks.
If the
KENTIK_LOG_FMT
environment variable is set tojson
, thenoutput a more structured log message.
Like so:
More examples: