slog tries to represent a time.Time without allocations, which involves
storing its UnixNanos value. But UnixNanos is undefined for some valid
times. Provide a fallback representation for those times by storing them
in the `any` field of `Value`.
Fixes#65902.
Change-Id: I736c739a92f77d7b1122ea0831524acdd2c4703f
Reviewed-on: https://go-review.googlesource.com/c/go/+/585519
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Plain string concatenation with the plus operator for Attr.String is
much faster than invoking fmt.Sprintf. Added a benchmark to verify
this (just running on my Mac with stuff in the background but should
be sufficient to demonstrate the effect).
name old time/op new time/op delta
AttrString-8 1.24µs ± 3% 0.43µs ± 0% -65.17% (p=0.000 n=20+17)
name old alloc/op new alloc/op delta
AttrString-8 432B ± 0% 152B ± 0% ~ (p=1.000 n=1+1)
name old allocs/op new allocs/op delta
AttrString-8 30.0 ± 0% 16.0 ± 0% ~ (p=1.000 n=1+1)
Change-Id: I18ac91cbff1047d168b51a595601e36b5f676615
Reviewed-on: https://go-review.googlesource.com/c/go/+/573517
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Emmanuel Odeke <emmanuel@orijtech.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Emmanuel Odeke <emmanuel@orijtech.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Code added in CL #558755 and CL #561315 (both via github) were not
appropriately formatted. Run gofmt to address this.
Change-Id: Iaf7abca9c2ad4e0fabecc417234b743b6b15e9c9
Reviewed-on: https://go-review.googlesource.com/c/go/+/564719
Run-TryBot: Joel Sing <joel@sing.id.au>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
adds a test case for the MarshalJSON and MarshalText method of the Level type in the slog package.
Change-Id: I3f79f0b46c41252ad9d743e03e34503e19998f3e
GitHub-Last-Rev: dab00d4c20
GitHub-Pull-Request: golang/go#65525
Reviewed-on: https://go-review.googlesource.com/c/go/+/561315
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
The new wording is slightly more strict than before in that
it guarantees each Write only contains exactly one Record,
while the previous wording opened up the possibility for
multiple Records in a Write call.
We also add a comment about the lack of sorting guarantees for
concurrently logged Records. That is, the obtained lock only covers
the Write call, rather than the combination of the call to time.Now,
JSON/text serialization, and also the Write call.
Change-Id: Ia65c50579215a35a1f5b2952c6954ddb60e7fe66
Reviewed-on: https://go-review.googlesource.com/c/go/+/563976
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Bypass: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TestValueEqual: Added test cases for DurationValue, StringValue, and TimeValue types.
TestValueAny: Added a test case for string values.
Change-Id: I74ef15bcfcacd0bf5d990debd24846283edf1ecd
GitHub-Last-Rev: 6adb9f6f31
GitHub-Pull-Request: golang/go#65303
Reviewed-on: https://go-review.googlesource.com/c/go/+/558755
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This commit is aimed at improving the readability and consistency
of the code base. Extraneous newline characters were present after
some return statements, creating unnecessary separation in the code.
Fixes#64610
Change-Id: Ic1b05bf11761c4dff22691c2f1c3755f66d341f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/548316
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
hex is in fact immutable, declare it as a const to avoid accidental
modification, also for consistency with other packages.
Change-Id: I88a6fed34d27ac57a0332dc7a27d3bf2ba0d6272
GitHub-Last-Rev: 08712be2d4
GitHub-Pull-Request: golang/go#64069
Reviewed-on: https://go-review.googlesource.com/c/go/+/541518
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
My hunch is the method was renamed during the development process but
the code comment was not updated to match.
Change-Id: Ib7aafc863f82a6bbe09e68dc3c5e087e16f228a9
Reviewed-on: https://go-review.googlesource.com/c/go/+/540536
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
pkgsite and go doc print the doc comment *after* the code, resulting in:
const (
LevelDebug Level = -4
...
)
Many paragraphs...
Names for common levels.
The "Names for common levels." feels out of place and confusing at the bottom.
This is also consistent with the recommendation for the first sentence in doc comments to be the "summary".
Change-Id: I656e85e27d2a4b23eaba5f2c1f4f811a88848c83
GitHub-Last-Rev: d9f7ee9b94
GitHub-Pull-Request: golang/go#61943
Reviewed-on: https://go-review.googlesource.com/c/go/+/518537
Reviewed-by: Alan Donovan <alan@alandonovan.net>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: qiulaidongfeng <2645477756@qq.com>
Reviewed-by: qiulaidongfeng <2645477756@qq.com>
Previously, the built-in handlers assumed a group was empty if and
only if it had no attributes. But a ReplaceAttr function that
returned an empty Attr could produce an empty group even if the group
had attrs prior to replacement.
The obvious solution, doing the replacement first and then checking,
would require allocating storage to hold the replaced Attrs. Instead,
we write to the buffer, and if no attributes were written, we back up
to before the group name.
Fixes#62512.
Change-Id: I140e0901f4b157e36594d8d476f1ab326f8f2c2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/529855
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
CL 525556 started using timeRE regexp to match time output from JSON
handler, and relaxed it to allow arbitrary (rather than fixed 3 digit)
precision.
What it missed is in JSON handler the fractional part is omitted
entirely (together with the decimal dot) when the nanoseconds field is
0.
As a result, there are occasional CI failures in js/wasm (which, I guess,
has better chances to return zero nanoseconds).
To fix the flaky test, let's use two different regular expressions,
tailored to text and JSON.
Change-Id: Ie98990fcf278bb0916ab31c9177e6b22a523062a
Reviewed-on: https://go-review.googlesource.com/c/go/+/530675
Run-TryBot: Kirill Kolyshkin <kolyshkin@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Reviewed-by: Andy Pan <panjf2000@gmail.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
The ReplaceAttr function should not see groups, only leaf attributes.
Previously, we checked an Value for being a group, then resolved it,
then called ReplaceAttr. We neglected to see if it was a group
after resolving it.
Now we resolve first, then check.
Fixes#62731.
Change-Id: I2fc40758e77c445f82deb2c9de8cae7a3b0e22cf
Reviewed-on: https://go-review.googlesource.com/c/go/+/530478
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
When slog.Record.Add(args) is called, with enough args to cause the
Record.back []Attr to be created, it is being created 1 too small, which
results in it immediately being grown again by append before the function
exits (needless allocation and copying).
This is because it is created with a capacity equal to countAttrs(args),
but forgets that there is an additional attribute to be appended: a
(args is just the remaining unconsumed attributes).
This PR fixes that by adding 1 to the capacity to account for the `a` attribute.
Additionally, when Record.back already exists, it will most likely be at
max capacity already. Rather than append to it and risk having it grown
multiple times, or grow too large, this adds a slices.Grow call to set it
to the right capacity, similar to what is already done in the
Record.AddAttrs(attrs) function.
Change-Id: Ic4bcf45909fe4436c586ccd2b8d61f24606b6be8
GitHub-Last-Rev: 4c924b610a
GitHub-Pull-Request: golang/go#62388
Reviewed-on: https://go-review.googlesource.com/c/go/+/524618
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
When I added this test in CL 514135, I missed the timeRE,
we should use it to make this test more precise.
Change-Id: I486f8e8bbbc3a17166107ef361d242ddf4ea2928
Reviewed-on: https://go-review.googlesource.com/c/go/+/525556
Run-TryBot: Andy Pan <panjf2000@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
There is no need to use atomic.Value, atomic.Pointer should also
be more performant here.
Change-Id: I04cc1befbe1112665a9d9e16197ea3caef5e5fa3
GitHub-Last-Rev: 577902519f
GitHub-Pull-Request: golang/go#62288
Reviewed-on: https://go-review.googlesource.com/c/go/+/523035
TryBot-Result: Gopher Robot <gobot@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Run-TryBot: Mateusz Poliwczak <mpoliwczak34@gmail.com>
In CL 421877 and CL 444278, time.Time.AppendFormat has been
specially optimized for the time.RFC3339Nano representation.
Relying on that optimization and modify the output to obtain the
fixed-width millisecond resolution that slog uses.
This both removes a lot of code and also improves performance:
name old time/op new time/op delta
WriteTime 93.0ns ± 1% 80.8ns ± 0% -13.17% (p=0.000 n=8+9)
Change-Id: I61e8f4476c111443e3e2098a45b2c21a76137345
Reviewed-on: https://go-review.googlesource.com/c/go/+/478757
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Change-Id: Ic4fcfe7335dab219790c19ded3bbb7265857404f
GitHub-Last-Rev: afc69c79b2
GitHub-Pull-Request: golang/go#62062
Reviewed-on: https://go-review.googlesource.com/c/go/+/519955
Run-TryBot: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Change-Id: Ib3daffb8a4cc018d62ed6e5741355b1c1a206034
Reviewed-on: https://go-review.googlesource.com/c/go/+/515775
Run-TryBot: Andy Pan <panjf2000@gmail.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
If the shared slice in a copied is modified, make a copy of it
and insert an attribute that warns of the bug.
Previously, we panicked, and panics in logging code should be avoided.
Change-Id: I24e9b0bf5c8cd09cf733e7dae8a82d025ef214e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/513760
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Fixes#61648
Change-Id: I6b7f4948ca89142a358d74624607daf42ea8b304
Reviewed-on: https://go-review.googlesource.com/c/go/+/514135
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Andy Pan <panjf2000@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
This extra newline causes pkg.go.dev and gopls to only show the bottom
half of this comment; I'm pretty sure this entire thing is meant to be
in the docs.
Change-Id: I5bbf081fb2072d9d773d5a995bc3693dc44f65ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/511855
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
This causes instances commonHandler created by withAttrs or withGroup to
share a mutex with their parent preventing concurrent writes to their
shared writer.
Fixes#61321
Change-Id: Ieec225e88ad51c84b41bad6c409fac48c90320b2
Reviewed-on: https://go-review.googlesource.com/c/go/+/509196
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Passing nil for a context is discouraged. We should avoid it.
Fixes#61219.
Change-Id: I664387070aacb56af580b6b0791ca40982d2a711
Reviewed-on: https://go-review.googlesource.com/c/go/+/508437
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Handlers should not display empty groups.
A group with no attributes is certainly empty. But we also want to
consider a group to be empty if all its attributes are empty groups.
The built-in handlers did not handle this second case properly.
This CL fixes that.
There are two places in the implementation that we need to consider.
For Values of KindGroup, we change the GroupValue constructor to omit
Attrs that are empty groups. A Group is then empty if and only if it
has no Attrs. This avoids a recursive check for emptiness.
It does require allocation, but that doesn't worry us because Group
values should be relatively rare.
For groups established by WithGroup, we avoid opening such groups
unless the Record contains non-empty groups. As we did for values, we
avoid adding empty groups to records in the first place, so we only
need to check that the record has at least one Attr.
We are doing extra work, so we need to make sure we aren't slowing
things down unduly. Benchmarks before and after this change show
minimal differences.
Fixes#61067.
Change-Id: I684c7ca834bbf69210516faecae04ee548846fb7
Reviewed-on: https://go-review.googlesource.com/c/go/+/508436
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Adds an optional close quote in the expected log message regex for TestConnections to prevent failing when the source filepath is surrounded in quotes due to it containing one or more spaces.
Fixes#61161
Change-Id: I0dd71fb4389bff963bbfdc668ef4e4dfe787eafc
Reviewed-on: https://go-review.googlesource.com/c/go/+/508055
Reviewed-by: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
It said that Attrs with an empty key are ignored.
In fact, zero Attrs are ignored.
Fixes#60870.
Change-Id: I221d3a25b0f0cc9001e06e9cc76bab29292c0741
Reviewed-on: https://go-review.googlesource.com/c/go/+/504535
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Add a shortlink in the doc to a guide to writing handlers, which is a work
in progress.
Change-Id: I1b01c90468382ffe53d9ad6f38253906e3f44857
Reviewed-on: https://go-review.googlesource.com/c/go/+/495920
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Previously, handleState.prefix was nil if and only if the length of
the prefix was zero. Now, prefix is never nil.
Fix the nil check in the code by also checking if the length is non-zero.
Change-Id: I9f69c0029cb1c73fe6c2919c78fee7d4085bfd85
Reviewed-on: https://go-review.googlesource.com/c/go/+/495977
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
It doesn't make sense to call Logger.WithGroup with the empty string.
Make it a no-op by returning the receiver.
This relieves handlers of the burden of detecting that case themselves.
Less importantly, but for consistency, if Logger.With is called with
no args, make it a no-op by returning the receiver.
Along the way, fix obsolete mentions of "the Logger's context" in the
doc.
Change-Id: Ia6caa4f1ca70c1c4b0cab3e222b2fda48be73fef
Reviewed-on: https://go-review.googlesource.com/c/go/+/496175
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Add the slog static analysis pass to `go vet`.
Vendor in golang.org/x/tools@master to pick up the pass.
Tweak a test in slog to avoid triggering the vet check.
Change-Id: I55ceac9a4e6876c8385897784542761ea0af2481
Reviewed-on: https://go-review.googlesource.com/c/go/+/496156
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
It's possible that the replacement for a built-in attribute is a Group.
That would cause a nil pointer exception because the handleState.prefix
field isn't set until later, in appendNonBuiltIns.
So create the prefix field earlier, at the start of commonHandler.handle.
Once we do this, we can simplify the code by creating and freeing the
prefix in newHandleState.
Along the way I discovered a line that wasn't being tested:
state.prefix.WriteString(h.groupPrefix)
so I modified an existing test case to cover it.
Change-Id: Ib385e3c13451017cb093389fd5a1647d53e610bf
Reviewed-on: https://go-review.googlesource.com/c/go/+/494037
Run-TryBot: Jonathan Amsterdam <jba@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
