Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

davidspek
Copy link
Collaborator

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 to github.com/bugsnag/bugsnag-go from an ancient version from 2014 to github.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.

@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.03% ⚠️

Comparison is base (6ccc551) 55.85% compared to head (f38dcc1) 55.83%.

❗ 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              
Files Changed Coverage Δ
configuration/configuration.go 65.10% <ø> (ø)
registry/registry.go 45.00% <0.00%> (-0.57%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: David van der Spek <vanderspek.david@gmail.com>
@milosgajdos
Copy link
Member

I need to mull over this for a bit...I think there is a better way to do this.

@thaJeztah
Copy link
Member

Honest question here, because I'm neither a "bugsnag" user, nor do I run a registry (beyond spinning up a quick registry for testing);

  • Is bugsnag something that would be commonly used (enough to make it an essential part of the standard feature-set provided by the registry?)
  • Are there ways to integrate with bugsnag outside of compiling in this package?
  • ^^ I know there's a lot of movement on integrating OTEL in many projects; could OTEL provide similar data when running a registry?

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 slog is becoming a thing, and I know various projects start to move away from logrus (or at least start to abstract things in preparation of being able to swap it for other solutions), so I'm wondering if there's alternatives we should look into.

@milosgajdos
Copy link
Member

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.

@davidspek
Copy link
Collaborator Author

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.

@milosgajdos
Copy link
Member

I think focussing on OTEL would be better than on particular 3rd party vendors 🤔

@thaJeztah
Copy link
Member

FWIW: I agree with all of the above, but I'm NOT against updating this dependency to be on a current version for the main branch.
While this code hasn't been ripped out, and the main branch targeting a major release ("v3"), it's better to ship with current dependencies than unmaintained ones ("now" is the moment to update dependencies, as we generally prefer not to risk doing so in patch releases).

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).

@thaJeztah
Copy link
Member

However, I expect users of either to disagree. Especially if we haven't provided an alternative like OTEL before we remove them.

These things are "tricky" indeed. Let me echo my thoughts;

  • main is a new major release, so breaking changes are allowed, including removal of features.
  • it's a bit unfortunate that this project was somewhat dormant for some time (although the code was quite stable and has been running in production in many high-profile services), a consequence was that there have not been many occasion s to do proper "deprecation notices" / announcements
  • but also realistically: the project gained a significant amount of features, which were the result of "drive-by contributions"; if these features are important to users / parties, then either those users / parties should step up and take on maintenance of the features, or (without anyone responsible for maintenance of them), the project to decide for them to be removed.

@davidspek
Copy link
Collaborator Author

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).

@deleteriousEffect
Copy link
Member

@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.

@davidspek
Copy link
Collaborator Author

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.

@milosgajdos
Copy link
Member

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.

@davidspek davidspek closed this Aug 18, 2023
This was referenced Aug 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants