You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently the Railtie logs the secret attributes hash as a string. In our Rails apps that use berkeley_library-logging (hereafter "BLL") for structured logging (e.g. Framework, AV Player), this produces entries like:
There's no good way to fix this with a vanilla Ruby or Rails Logger, but Ougai (which BLL is built on) supports passing optional exception and data arguments along with the message (and is smart enough to tell when the exception argument is omitted), so we can say
Off the top of my head, I have a couple of ideas as to how to approach this:
Add BLL as a dependency and use BL::Logging.logger (which in a Rails app that pulls in the BLL railtie will also be the Rails logger, allowing us to replace this code -- which could be as simple as including BL::Logging in BL::Docker::Logging)
Do something like defined?(Ougai::Logger) && logger.instance_of?(Ougai::Logger) to check whether we are, and if not, fall back to the inspect- based approach
Try to do something clever involving reflection to figure out whether the logger's info method accepts more than one parameter
Just assume we're using BLL or another Ougai-based log framework
Of these, I prefer (1) (but then, I would say that, wouldn't I?).
Without the explicit BLL dependency, (2) feels a little arbitrary — there are other Ruby structured logging frameworks out there, like Logcraft -- which also accepts additional data, but doesn't use the same syntax -- so what's special about Ougai? Plus we're depending on the syntax/semantics of a certain version of a dependency without making that dependency explicit anywhere, which feels risky. (The same would go for checking for BLL without actually depending on it.)
I'm not enamored of (3) since reflection can't tell us anything about the semantics of the method; it feels like just trying to do (2) without admitting it.
(4) would work with our apps, but feels risky: again, we're depending on the semantics of an undeclared dependency — which in this case wouldn't even be obvious when looking at the code — which if it fails will fail in a non-obvious way.
The text was updated successfully, but these errors were encountered:
Currently the Railtie logs the secret attributes hash as a string. In our Rails apps that use
berkeley_library-logging
(hereafter "BLL") for structured logging (e.g. Framework, AV Player), this produces entries like:There's no good way to fix this with a vanilla Ruby or Rails
Logger
, but Ougai (which BLL is built on) supports passing optionalexception
anddata
arguments along with the message (and is smart enough to tell when theexception
argument is omitted), so we can sayand get nice structured messages like:
Off the top of my head, I have a couple of ideas as to how to approach this:
BL::Logging.logger
(which in a Rails app that pulls in the BLL railtie will also be the Rails logger, allowing us to replace this code -- which could be as simple as includingBL::Logging
inBL::Docker::Logging
)defined?(Ougai::Logger) && logger.instance_of?(Ougai::Logger)
to check whether we are, and if not, fall back to theinspect-
based approachinfo
method accepts more than one parameterOf these, I prefer (1) (but then, I would say that, wouldn't I?).
Without the explicit BLL dependency, (2) feels a little arbitrary — there are other Ruby structured logging frameworks out there, like Logcraft -- which also accepts additional data, but doesn't use the same syntax -- so what's special about Ougai? Plus we're depending on the syntax/semantics of a certain version of a dependency without making that dependency explicit anywhere, which feels risky. (The same would go for checking for BLL without actually depending on it.)
I'm not enamored of (3) since reflection can't tell us anything about the semantics of the method; it feels like just trying to do (2) without admitting it.
(4) would work with our apps, but feels risky: again, we're depending on the semantics of an undeclared dependency — which in this case wouldn't even be obvious when looking at the code — which if it fails will fail in a non-obvious way.
The text was updated successfully, but these errors were encountered: