Conversation
b88e62b to
b384b24
Compare
b384b24 to
99870e5
Compare
| end | ||
|
|
||
| # @!visibility private | ||
| def logger |
There was a problem hiding this comment.
Not blocking: Although we marked it as private in docs, it's still very accessible to users. So I'd suggest keeping it like:
def logger
print_deprecation_message_to_ask_users_not_to_use_it
configuration.sdk_logger
endThen at least some users of it can have time to move off of it.
There was a problem hiding this comment.
@st0012 so, this is tricky - in #2600 we're adding Sentry.logger interface, so it is not going to be deprecated, but it will be enabled conditionally based on config._experiments[:enable_logs]. What we could do is printing deprecation message only if enabled_logs is not true - WDYT? The message would say something like "Starting with 5.{version_where_we_enable_logging_by_default} Sentry.logger will become public API for structured logging feature"
There was a problem hiding this comment.
sorry late to the convo, we can also go for Sentry::Logger with module level methods instead of Sentry.logger if we want to keep the old one unchanged..
There was a problem hiding this comment.
@sl0thentr0py I think there's no way around that - IF there are people who use Sentry.logger, then we gotta let them know via deprecation warning that this logger is changing its purpose. I'm not sure how changing to Sentry::Logger class methods would help?
There was a problem hiding this comment.
we still deprecate but all the new stuff would go to Sentry:: Logger and Sentry.logger would stay what it was for now till next major
There was a problem hiding this comment.
@sl0thentr0py oh I see. So instead of config.sdk_logger we switch to global Sentry::Logger and deprecate original config.logger behavior. Yeah this makes perfect sense. It would simplify LoggingHelper and a bunch of constructors. One question though - in sentry-rails we actually set the logger to a duped Rails.logger, what would happen with this if we want to switch to Sentry::Logger and remove configurable logger?
There was a problem hiding this comment.
ok let me be clearer,
about the internal SDK logging:
config.loggerstays but deprecatedconfig.sdk_loggercomes and replacesconfig.logger
--
Now, everything about the new Sentry Logging Feature goes under a completely new namespace Sentry::Logging.do_something (not Sentry::Logger since that's also taken) and NOT Sentry.logger.do_something
There was a problem hiding this comment.
Now, everything about the new Sentry Logging Feature goes under a completely new namespace Sentry::Logging.do_something (not Sentry::Logger since that's also taken) and NOT Sentry.logger.do_something
I don't think it's a good idea as it would diverge from the naming convention from other SDKs where we already have Sentry.logger - also, it's just much nicer and intuitive vs Sentry::Logging.
What would be the benefit of going with Sentry::Logging? The way I see it is that we just need to deal with the deprecation cycle of config.logger / Sentry.logger being used as an internal logging facility and eventually replace it with the new logging stuff.
Please remember that initially we are going to release new logging with _experiments[:enable_logger] config option, so the users will have to make a deliberate choice to enable it and we can simply add docs/release notes + maybe even extra warning on init, that say that when this is enabled, then config.logger / Sentry.logger is no longer the same thing and if the user relies on it, then they should basically stop doing that as it was always meant to be used solely as an internal SDK logger.
There was a problem hiding this comment.
resolved in slack, we will show deprecation and do the switch based on the config
There was a problem hiding this comment.
@st0012 @sl0thentr0py I re-added Sentry.logger method via cb55544 and it has a deprecation warning now:
99870e5 to
3fe77a6
Compare
We won't use it after all
Addresses this warning: > [sentry] `config.logger=` is deprecated. Please use `config.sdk_logger=` instead. getsentry/sentry-ruby#2621
This is in preparation for #2604 - we have an internal
Sentry.loggerused for warn/error/debug logging with the SDK but with Structured Logging feature we're introducing a public-facingSentry.loggerAPI, so our internal one needs to be renamed.