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.

Sign up for GitHub

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

Jump to bottom

Add compatibility with sidekiq 7.0 #546

Merged
merged 8 commits into from
Aug 7, 2023
Merged

Add compatibility with sidekiq 7.0 #546

merged 8 commits into from
Aug 7, 2023

Conversation

kevinnio
Copy link
Contributor

@kevinnio kevinnio commented Jul 6, 2023

This adds a new mock loader that allows for testing against sidekiq 7.0. No actual code changes were needed to pass all the tests.

This also adds the relevant appraisal.

Included is a commit that removes some appraisals since they were effectively targeting the same sidekiq version.

@kevinnio
Make gem compatible with sidekiq 7.0
5bcd76e 
This adds a new mock loader that allows for testing against sidekiq 7.0.
This also the relevant appraisal.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jul 6, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@kevinnio kevinnio changed the title Sidekiq add compatibility with sidekiq 7.0 Add compatibility with sidekiq 7.0 Jul 6, 2023
@kevinnio
Simplify appraisals
cd330a8 
This removes some appraisals since they're efectively targeting the same
sidekiq version.

Namely, sidekiq-6.0, sidekiq-6.1, sidekiq-6.3 and sidekiq-6.4 were actually
resolving to 6.5.2.

With this change, we run less tests but we actually target different
sidekiq versions.
@kevinnio kevinnio force-pushed the sidekiq-add-compatibility-with-sidekiq-7.1 branch from 401ab10 to cd330a8 Compare July 6, 2023 02:43
@kevinnio kevinnio mentioned this pull request Jul 6, 2023
Closed
@arielvalentin
Copy link
Collaborator

@kevinnio looks like the test suite never finishes and hangs at some point.

Would you mind taking a look?

arielvalentin
arielvalentin requested changes Jul 6, 2023
View reviewed changes
Copy link
Collaborator

@arielvalentin arielvalentin left a comment

Choose a reason for hiding this comment

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

Waiting on tear suite to complete successfully

@stevenharman stevenharman mentioned this pull request Jul 6, 2023
Closed
@kaylareopelle
Copy link
Contributor

kaylareopelle commented Jul 6, 2023 •
edited
Loading

Hi @kevinnio, Sidekiq 7.0 introduced a change to the middleware. Essentially, you're supposed to include some modules now in your middleware classes.

Did you look into adding this middleware? It might not be necessary for OTel's instrumentation. At New Relic, we needed it to keep our tests passing, but I don't quite recall why. We added it conditionally to keep the instrumentation backward compatible.

This might be why the redis/sidekiq tests have some issues? It looks like config.redis is called in the test helper. I think that's only accessible in 7.0 with the new middleware.

@kevinnio
Copy link
Contributor Author

kevinnio commented Jul 6, 2023 •
edited
Loading

Hi, @kaylareopelle. Yes, I am aware of those new modules. Sidekiq::ServerMiddleware and Sidekiq::ClientMiddleware are currently only shortcuts for some common used methods. We don't call any of those methods in the instrumentation, so we don't need them right now. Nevertheless, I can include them just in case they change their behavior in future releases. Thanks for the advise!

I'll also take a look at why the specs are not passing.

@kevinnio
Add Sidekiq 7.0 middleware modules
80a0913 
Sidekiq 7.0 requires middlewares to add new modules. These modules are
not currently in use by our code, but I'm adding them for good measure.
@kevinnio kevinnio force-pushed the sidekiq-add-compatibility-with-sidekiq-7.1 branch from 1a99365 to 80a0913 Compare July 6, 2023 17:35
@kevinnio
Fix specs for sidekiq 6.4.2 and below
d75d93a 
Those appraisals need an older version of the redis gem so sidekiq can
establish a connection with the redis server properly. Without it, tests
against sidekiq 6.4.2 hang for hours before timing out in GitHub
Actions.
@kevinnio
Copy link
Contributor Author

kevinnio commented Jul 6, 2023

Pushed a fix for the specs.

@kevinnio
Copy link
Contributor Author

@arielvalentin Specs are now fixed.

@kevinnio kevinnio requested a review from arielvalentin August 4, 2023 00:41
@kevinnio kevinnio force-pushed the sidekiq-add-compatibility-with-sidekiq-7.1 branch from a815312 to e0c20fe Compare August 4, 2023 00:43
@arielvalentin arielvalentin merged commit cd54749 into open-telemetry:main Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertlaurin robertlaurin robertlaurin approved these changes

@arielvalentin arielvalentin arielvalentin approved these changes

@fbogsany fbogsany Awaiting requested review from fbogsany fbogsany is a code owner

@mwear mwear Awaiting requested review from mwear mwear is a code owner

@dazuma dazuma Awaiting requested review from dazuma dazuma is a code owner

@ericmustin ericmustin Awaiting requested review from ericmustin ericmustin is a code owner

@ahayworth ahayworth Awaiting requested review from ahayworth ahayworth is a code owner

@plantfansam plantfansam Awaiting requested review from plantfansam plantfansam is a code owner

@robbkidd robbkidd Awaiting requested review from robbkidd robbkidd is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kevinnio @arielvalentin @kaylareopelle @robertlaurin