Zap + logr == not very expressive, open to fixes?

[thockin]

One of my side-projects is https://github.com/go-logr/logr (an API for libraries to use to log) and alongside it lives the bridge to zap (https://github.com/go-logr/zapr).

Logr uses integer log levels - V(3) is "more debuggy" than V(2), which is more than V(1). Zap doesn't natively support that, but it seems to work, with a small number of caveats. So far I have found 1, actually.

zapcore/sampler.go does an indexed lookup by log level. When I use Logr verbose levels > 1 (which maps to Debug in zap) I get an out-of-bounds panic.

A quick hack of adding some number of extra debug levels (just add 100 indices to that array and accesses by +100) makes a trivial test pass with V(2), V(3), etc.

Before I really dig in, I wanted to see if this is something zap maintainers are willing to entertain ? I imagine it would ultimately manifest as a set of small changes here and there to handle the extra levels, plus tests. E.g. the level name already renders as "Level(-2)", so someone thought about this a little already.

[prashantv]

I don't think we want to add an arbitrary number of levels to the sampler array, since that's semi-support for custom levels (which aren't actually supported), and it works in some cases (if level is below some max value). There's also a slight cost for something unsupported.

However, I think avoiding panic is reasonable, so one option that we're open to is doing a "in-bounds" check before accessing the counts. We could then fail-open for unsupported levels, so they are not sampled.

What do you think of that approach?

[thockin]

SGTM. Do you want a PR or do it yourself?

…

On Mon, Jun 28, 2021, 5:20 PM Prashant Varanasi ***@***.***> wrote: I don't think we want to add an arbitrary number of levels to the sampler array, since that's semi-support for custom levels that works in some cases (if level is below some max value), and adds a slight cost. However, I think avoiding panic is reasonable, so one option that we're open to is doing a "in-bounds" check before accessing the counts. We could then fail-open for unsupported levels, so they are not sampled. What do you think of that approach? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#972 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVFO4FVM5HXUXE7V6UDTVEGVLANCNFSM47OIZSVA> .

[prashantv]

PR would be great, thanks!