pkg/logger/lk: new package for log key glossary

[jmank88]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

Supports:

• bump common; use log key package chainlink#15936

[pavel-raykov]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

What is the intended use of the keys from logkey? Is this for labelling with ZAP's zap.String(key, value) construction, or is it for arbitrary string / int constants that can be used for logging in a centralized way?

[jmank88]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

What is the intended use of the keys from logkey? Is this for labelling with ZAP's zap.String(key, value) construction, or is it for arbitrary string / int constants that can be used for logging in a centralized way?

We don't typically use the zap field API like that, but that would be valid. More often we just call the w variants: Logger.<level>w("<message>", logkey.ChainID, chainID)

[pavel-raykov]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

What is the intended use of the keys from logkey? Is this for labelling with ZAP's zap.String(key, value) construction, or is it for arbitrary string / int constants that can be used for logging in a centralized way?

We don't typically use the zap field API like that, but that would be valid. More often we just call the w variants: Logger.<level>w("<message>", logkey.ChainID, chainID)

I see, so are you planning to extract all the possible keys in some kind of automatic way? Or would you just keep adding them here continuously?

[jmank88]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

What is the intended use of the keys from logkey? Is this for labelling with ZAP's zap.String(key, value) construction, or is it for arbitrary string / int constants that can be used for logging in a centralized way?

We don't typically use the zap field API like that, but that would be valid. More often we just call the w variants: Logger.<level>w("<message>", logkey.ChainID, chainID)

I see, so are you planning to extract all the possible keys in some kind of automatic way? Or would you just keep adding them here continuously?

We could try to start with some automation, but adding keys manually would be sufficient for most common inconsistencies.

I did consider providing zap field support as well, but I'm not sure how that should work. Even just ChainID is an interesting example. We probably would want to encourage (or even enforce) string based chain IDs, but we don't want to force callers to make a string to pass in, since that is inefficient in cases where the log message does not need to be serialized. We'd rather also accept fmt.Stringer. So zapfield.ChainID("string") isn't flexible enough, we'd want a fmt.Stringer version, and probably int64 too. It wasn't obvious to me how to sort that out in a clean way, so I left it out of scope for now.

[pavel-raykov]

This PR introduces a new package logkey to serve as a glossary of logger keys in order to align common fields.

What is the intended use of the keys from logkey? Is this for labelling with ZAP's zap.String(key, value) construction, or is it for arbitrary string / int constants that can be used for logging in a centralized way?

We don't typically use the zap field API like that, but that would be valid. More often we just call the w variants: Logger.<level>w("<message>", logkey.ChainID, chainID)

I see, so are you planning to extract all the possible keys in some kind of automatic way? Or would you just keep adding them here continuously?

We could try to start with some automation, but adding keys manually would be sufficient for most common inconsistencies.

I did consider providing zap field support as well, but I'm not sure how that should work. Even just ChainID is an interesting example. We probably would want to encourage (or even enforce) string based chain IDs, but we don't want to force callers to make a string to pass in, since that is inefficient in cases where the log message does not need to be serialized. We'd rather also accept fmt.Stringer. So zapfield.ChainID("string") isn't flexible enough, we'd want a fmt.Stringer version, and probably int64 too. It wasn't obvious to me how to sort that out in a clean way, so I left it out of scope for now.

Sound good. Thanks for explaining!

[jmank88]

Perhaps lowercase is needlessly strict? I copied this from a type FeedID string.

[pavel-raykov]

I think it is fine either way. What exactly do you mean by strict? I.e., in which situation lower case is better/worse than the upper case?

[jmank88]

I mean that it seems superficial to expect/enforce that we use only lowercase. It is trivial to convert to only lowercase at the call site if that is what you need, and it could be a big pain to chase around all the cases and align them as lower case. Perhaps filtering logs is easier with all lowercase? That might be worth it.

[jmank88]

Is logkey the best package name? We might want to use these with Prometheus too, and so we could generalize and shorten to just keys:

Suggested change

	lggr.Errorw("could not convert %s to feedID", logkey.FeedID, feedID)
	lggr.Errorw("could not convert %s to feedID", keys.FeedID, feedID)

But this would have name collisions in some places.

Something like labels is perhaps less collision prone, but more verbose:

Suggested change

	lggr.Errorw("could not convert %s to feedID", logkey.FeedID, feedID)
	lggr.Errorw("could not convert %s to feedID", labels.FeedID, feedID)

Any other ideas?

[pavel-raykov]

logconst or logentry?

[jmank88]

I think it would need to be fewer than 6 characters to be an improvement on logkey

[pavel-raykov]

logval or logdef ?

[jmank88]

Would package lk be too opaque? Typically they should be meaningful, but in this case the usage should always make it clear 🤔 It would also make aliasing packages less obnoxious (chainlk, caplk, etc.)

[jmank88]

Update to package lk to try it out. I think I like it.

[krehermann]

discussed offline.

to move forward, agree to keep this simple and audit the growth/ussage of the package with intent to mv/refactor if it becomes too ungainly