util/mon: add monitor address to logging messages

[yuzefovich]

This will allow us to uniquely identify all messages coming from
a single monitor. Additionally, this commit fixes the redactability of
the file name and function name when getting a stack trace.

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

I tried to find a way to print the pointer address in the redact API and didn't find anything better than redact.Safe. I'm assuming that pointer addresses should be safe to not redact, right?

I also noticed the stack traces weren't printed. Now the output looks like

I220408 18:38:58.692380 1142 util/mon/bytes_usage.go:801 ⋮ [n1,server drain process,intExec=‹×›] 496  internal SQL executor (0xc001e21520): now at 0 bytes (-10240) - bytes_usage.go:844:util/mon.(*BytesMonitor).releaseBudget,bytes_usage.go:436:util/mon.(*BytesMonitor).doStop,bytes_usage.go:409:util/mon.(*BytesMonitor).Stop,conn_executor.go:1149:sql.(*connExecutor).close,internal.go:214:sql.(*InternalExecutor).initConnEx.func1

when redaction is enabled.

Without the fix to the stack traces it is:

I220408 18:36:10.013799 1909 util/mon/bytes_usage.go:801 ⋮ [n1,client=127.0.0.1:62682,user=root] 102  session root (0xc001e32000): now at 0 bytes (-10240) - ‹×›:844:‹×›,‹×›:436:‹×›,‹×›:409:‹×›,‹×›:234:‹×›,‹×›:499:‹×›

[knz]

Sorry I did not see this before!

The change looks good, but you still need a unit test.

I'm assuming that pointer addresses should be safe to not redact, right?

It's not for the redact library to decide -- whether e.g. a pointer is nil or not can be sensitive information.
We could arguably decide that pointers are always safe in cockroachdb (via a new API in redact to declare pointers as safe). If you find this important, please file an issue on the redact repo.

Reviewed 1 of 2 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @yuzefovich)

---

pkg/util/mon/bytes_usage.go line 310 at r1 (raw file):

		settings:             settings,
	}
	m.nameWithPointer = redact.Sprintf("%s (%p)", name, redact.Safe(m))

Have you tried without redact.Safe?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @yuzefovich)

---

pkg/util/mon/bytes_usage.go line 310 at r1 (raw file):

Previously, knz (kena) wrote…

Have you tried without redact.Safe?

Oh sorry, this comment was before I read yours. Please ignore. Your code is fine.

[yuzefovich]

No worries! Your review is actually quite timely since we have another investigation that might benefit from this (from the monitor address part).

What unit test do you have in mind?

I'm assuming that pointer addresses should be safe to not redact, right?

It's not for the redact library to decide...

I meant the question only in general, i.e. "the pointer address is not PII, so it is safe to not redact, right?", I don't think it's needed to push this into redact library.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @yuzefovich)

[knz]

What unit test do you have in mind?

Check that the pointer is included in the error messages.

[yuzefovich]

Note that the pointer is only included into the logging messages and not included into the error messages. I imagine to use this only as a debugging tool when tracking down memory accounting leaks, and in that use case we do enable verbose logging for bytes_usage file. At the same time, I don't want to make the error messages confusing or scary to users, so I think it's better to not include the pointer there. Thoughts?

In terms of the test - I feel like it's not worth it to do the effort of setting something up - this would require analysis of the trace and would probably be quite heavy-weight. Since this is an internal use case, manual one time QA I did already to make sure that the pointer does show up in the logs seems sufficient to me.

[michae2]

Reviewed 1 of 2 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

[yuzefovich]

@knz do I have your blessing? :)

[knz]

Reviewed 2 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @yuzefovich)

[yuzefovich]

TFTRs!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed:

• GitHub CI (Cockroach)

[yuzefovich]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[yuzefovich]

blathers backport 22.1

[yuzefovich]

blathers backport 21.2

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 1fd84d7 to blathers/backport-release-21.2-79682: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2 failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}