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

Remove uses of metriks and its dependencies. #283

Merged
merged 3 commits into from
Jul 21, 2020
Merged

Conversation

sistawendy
Copy link
Contributor

The atomic gem, which is a dependency of metriks, has started breaking builds in Ruby 2.4.1. This pull request removes the only use of metriks and its dependencies in this repo, which is an optional middleware.

Copy link

@phil-hildebrand phil-hildebrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks ok @sistawendy . Can you add the test results? (I'll validate that as well locally).

@phil-hildebrand
Copy link

phil-hildebrand commented Jul 21, 2020

Actually, I think there will need to be some changes if metriks is removed.

When a job runs, it appears to call around_perform which currently expects Metriks

Did the tests all pass with your change?

@phil-hildebrand
Copy link

phil-hildebrand commented Jul 21, 2020

Maybe you could just take out the require and stub the metriks functions so that middleware still works, it just doen't time the jobs?

@phil-hildebrand
Copy link

@b4hand / @danlecocq would either of you be available to weigh in and/or make suggestions?

@sistawendy
Copy link
Contributor Author

If you don't tell Rack to use the middleware, as Barbosa doesn't, the middleware never gets called. Neutering the middleware is an option. Did I mention that we have this branch deployed to production right now?

The tests, by the way, are a disaster.

@phil-hildebrand
Copy link

Yeah - that make sense. I do think for those that may use it (middle ware) outside of moz, it might make sense just to stub it out so that if they happen not to have the version pinned, it doesn't blow up.

I hadn't tried to run the tests yet, so if there are a bunch failing already, I'd agree barbosa in prod is a valid test case 👍

Copy link

@phil-hildebrand phil-hildebrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with the one suggestion/consideration

@sistawendy
Copy link
Contributor Author

Hollowed-out metriks middleware added.

@sistawendy sistawendy merged commit 7ac9f3a into master Jul 21, 2020
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

2 participants