instana / ruby-sensor

💎 Ruby Distributed Tracing & Metrics Sensor for Instana
https://www.instana.com/
MIT License
26 stars 25 forks source link

Add support for Sidekiq 7.0 #277

Closed jeremycw closed 1 year ago

jeremycw commented 1 year ago

Sidekiq 7.0 now depends on the redis-client gem instead of the redis gem. This caused issues with sidekiq-client.rb and sidekiq-worker.rb

Ferenc- commented 1 year ago

Hi @jeremycw, Thank you for your interest in contributing!

I would like to kindly ask you to sign your commit with git commit -s, indicating that you as the submitter accept the Developer's Certificate of Origin 1.1, and submit this work under our MIT License which grants the usual MIT License rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell to anyone.

github-actions[bot] commented 1 year ago

❌ @jeremycw the signed-off-by was not found in the following 1 commits:

📝 What should I do to fix it?

All proposed commits should include a sign-off in their messages, ideally at the end.

❔ Why it is required

The Developer Certificate of Origin (DCO) is a lightweight way for contributors to certify that they wrote or otherwise have the right to submit the code they are contributing to the project. Here is the full text of the DCO, reformatted for readability:

By making a contribution to this project, I certify that:

a. The contribution was created in whole or in part by me and I have the right to submit it under the open source license indicated in the file; or

b. The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file; or

c. The contribution was provided directly to me by some other person who certified (a), (b) or (c) and I have not modified it.

d. I understand and agree that this project and the contribution are public and that a record of the contribution (including all personal information I submit with it, including my sign-off) is maintained indefinitely and may be redistributed consistent with this project or the open source license(s) involved.

Contributors sign-off that they adhere to these requirements by adding a Signed-off-by line to commit messages.

This is my commit message

Signed-off-by: Random Developer <randomdeveloper@example.com>

Git even has a -s command line option to append this automatically to your commit message:

$ git commit -s -m 'This is my commit message'
jeremycw commented 1 year ago

Hey @Ferenc- I've made some updates based on your suggestions. I've left this functionality where it is for now since in it's current incarnation it's very specific to Sidekiq requiring access to the client through Sidekiq.redis_pool and the client object in this case is actually an instance of Sidekiq::RedisClientAdapter::CompatClient

Ferenc- commented 1 year ago

Thank you for making the changes. In test/instrumentation/sidekiq-client_test.rb we have a number of test cases. When running those on this PR branch with Sidekiq 7.0 the following appear to be failing :

  1. test_enqueue_failure
  2. test_successful_worker_starts_new_trace
  3. test_failed_worker_starts_new_trace
  4. test_failed_worker_continues_previous_trace
  5. test_successful_worker_continues_previous_trace

And with earlier versions (6.0, 5.0, 4.2) all the above and one more:

  1. test_enqueue
Ferenc- commented 1 year ago

Hi,

Thank you for the initiative. We believe that Sidekiq 7.0 support is complete now, so I am closing this for now. We can reopen if needed.

daande commented 1 year ago

@Ferenc- So we have been running on a forked version of this gem that @jeremycw implemented support for Sidekiq 7.0 on. We recently spent some time trying out the latest release of this gem and can confirm that traces from sidekiq 7.0 are not showing up in instana. We believe that sidekiq 7.0 is not supported, can you please reopen and investigate?

Ferenc- commented 1 year ago

I have created the AHA FR for the full support: https://bigblue.aha.io/ideas/ideas/INSTANA-I-1959