New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Shopify/logrus-bugsnag and bugsnag-go to v2 #3991
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #3991 +/- ##
==========================================
- Coverage 55.85% 55.83% -0.03%
==========================================
Files 110 110
Lines 11069 11073 +4
==========================================
Hits 6183 6183
- Misses 4193 4197 +4
Partials 693 693
☔ View full report in Codecov by Sentry. |
Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
934c397
to
a8bd8f7
Compare
I need to mull over this for a bit...I think there is a better way to do this. |
Honest question here, because I'm neither a "bugsnag" user, nor do I run a registry (beyond spinning up a quick registry for testing);
Reason I'm asking is that I know we want to reduce some feature-bloat that this repository has acquired over the years, to save maintenance, but also because |
I have been wondering the same thing, especially in light of another PR that updates NewRelic integration. These things are useful to folks running registry in some sort of production setup but are not required for running registry or related to any open standards. It's clear we are missing some sort of plug-in [middleware] plumbing or some such - the same thing with storage drivers. Something to think about before we merge this. |
I'm also not a bugsnag user, just wanted to update the dependency to something not ancient to cleanup the code a bit. I'm pretty sure all it's doing is shipping the log lines of the registry to bugsnag, which in theory could also be replaced by using OTEL and shipping them to an OTEL receiver, which then would forward to bugsnag (I assume this exists, or they are doomed to fail). I would be all for ripping this out, which means we should probably also remove NewRelic. However, I expect users of either to disagree. Especially if we haven't provided an alternative like OTEL before we remove them. |
I think focussing on OTEL would be better than on particular 3rd party vendors 🤔 |
FWIW: I agree with all of the above, but I'm NOT against updating this dependency to be on a current version for the But we should have a discussion / decision on what to do with these kind of features, especially with "the world" converging towards OTEL, and most (all?) other options may be too opinionated to consider including as a "default". With a limited number of maintainers (and bandwidth among those), there's only so many options we can reasonably "support" (or be comfortable with supporting). But yes, ideally we would have things more pluggable (or an alternative approach) so that those that really need these features can use them (without being forced to maintain a fork of the code just for that). |
These things are "tricky" indeed. Let me echo my thoughts;
|
I'm reviving the OTEL integration in #3996 but I don't want that to block the v3 release. So I think we should upgrade what we currently have on main so that is at least up to date. Then if it's possible to get OTEL over the line for v3 we can remove them again. Otherwise there can be an overlap where OTEL, bugsnab and newrelic are all supported and the latter 2 can get removed in an upcoming release (which hopefully this time will be relatively soon following v3). |
@davidspek moving to v3 we have an opportunity to remove this now with the option of putting it back in if and only if there's truly a user base out there for this. It's not ideal if we have to re add it, but it's better than including it in perpetuity. |
That's also an option. Which does ensure the problem is fixed without the possibility of things getting in the way. I'll prepare 2 PRs to remove them tomorrow so either way we have the pieces in place on time for v3. |
I am also in favour of chucking both Bugsnag and NewRelic dependencies. I understand the pros and cons of their being and not being in the codebase but v3 is an opportunity to clean up this codebase from third-party dependencies and outdated cruft which, in the grand scheme of things, only add burden to the project. Until we have a good way of dealing with plugins we should consider proper yak shave. |
This PR updates
github.com/Shopify/logrus-bugsnag
from a very old version the Go package registry doesn't even know about to the latest version. Along with that came the update togithub.com/bugsnag/bugsnag-go
from an ancient version from 2014 togithub.com/bugsnag/bugsnag-go/v2
, which required a slight change in our configuration. Due to the configuration change we should try to get this merged before v3.