cobrautil v2 #19
Merged
cobrautil v2 #19
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vroldanbet
force-pushed
the
cobrautil-v2
branch
3 times, most recently
from
6e2b011 to
8bfdabf
Compare
this allows clients importing just the bits they need without bringing all the transitive module dependencies
the functionality is largely the same as v1. The gist of this redesign is configurability: v1 didn't offer any API to have cobrautil consumer adjust the logger was configured. This is important as zerolog provides a vast API with many knobs to tweak. - a new fluent API is offered so that cobrautil/zerolog can be extended with options as the need arises. It's used to implement two new options in the package - v1 assumed the consumer would use the zerolog/log global instance package. This forces the consuming application to use that global instance. The WithTarget option offers a callback when the logger is instantiated, so e.g. consumers can cache the instance somewhere else - v1 used zerolog global level. This is an unexpected side-effect the consumer may not want, specially given cobrautil is creating an instance of a zerolog Logger. The implementation now adjusts the level on the logger instance instead. - singleton methods are provided to offer an easy path of migration to v2. - WithAsync configures the logger to be non-blocking, implemented using diode buffered-ring. This is default by default to retain v1 behaviour.
behaviour should be the same as v1, but: - removes dependency on zerolog and replaces with go-log/logr - allows defining the logger used by otel
vroldanbet
force-pushed
the
cobrautil-v2
branch
from
8bfdabf to
6eeae43
Compare
vroldanbet
force-pushed
the
cobrautil-v2
branch
from
6b41b83 to
8627e24
Compare
- since it's optional and has a sensible default - also align preRunLevel in OTel with zerolog
- turned all optional arguments into a ConfigureFunc - added WithHandler to provide a http.Handler to inject into ServerFromFlags - added ListenFromFlags that instantiates the http.Server - migrated go go-log/logr facade
vroldanbet
force-pushed
the
cobrautil-v2
branch
from
a2352c4 to
553eadd
Compare
- turned all optional arguments into a ConfigureFunc - migrated go go-log/logr facade
- make sure messaging is consistent - make use of same logging level across packages - do not warn on insecure http/gRPC. Instead load additional key-value - log scheme in http package
jzelinskie
reviewed
Oct 3, 2022
| } | ||
|
|
||
| // CobraUtil carries the configuration for a otel CobraRunFunc | ||
| type CobraUtil struct { |
There was a problem hiding this comment.
I'd name this type config in each package instead of cobrautil. I think it makes sense to also make the type private, since all of the fields are also private. I'm pretty sure you can still return the type from functions, but folks will just be unable to construct it from a literal in other packages.
Picking up optgen might also be a good idea for this too: https://github.com/ecordell/optgen
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #17
Main design choices
Zerolog
The main problems is v1 didn't offer any API to have
cobrautilconsumers adjust how the logger was configured. This is important as zerolog provides a vast API with many knobs to tweak.cobrautil/zerologcan be extended with options as the need arises.zerolog/logglobal instance package. This forces the consuming application to use that global instance. TheWithTargetoption offers a callback when the logger is instantiated, so e.g. consumers can cache the instance somewhere else. It continues to default to that global instance.Logger. The implementation now adjusts the level on the logger instance instead.WithAsyncconfigures the logger to be non-blocking, implemented using diode buffered-ring. This is default by default to retain v1 behaviour.See how it's used in https://github.com/authzed/spicedb/pull/844/files#diff-df0635588ca5d6a577487faacc185c3838a3fe2b321660c36372541797c54c74R59-R67
OTel
zerologand replaces withgo-log/logrWithLoggermakes it possible to set the global logger used by the OTel librarydebugHTTP
WithLoggerallows injecting a customgo-log/logrvalueWithHandlerallows injecting ahttp.Handlerinto thehttp.Serverto be createddebuginstead ofinfo. Can be tweaked withWithPreRunLevelwarnlevel logging for non-https configuration moves topreRunLevel. Insecure http is used behind load-balancers that do TLS offloading.gRPC
WithLoggerallows injecting a customgo-log/logrvaluedebuginstead ofinfo. Can be tweaked withWithPreRunLevelwarnlevel logging for non-https configuration moves topreRunLevel. Insecure http is used behind load-balancers that do TLS offloading.