Add support for the `kicks` gem #4253

marcotc · 2025-01-02T23:50:05Z

Kicks is a fork/continuation of the sneakers gem (announcement).

Change log entry
Yes. Add support for the kicks gem

Additional Notes:

They kept everything the same, even the file paths (require 'sneakers') and module names (::Sneakers).

I created an alias for instrument :kicks to instrument :sneakers, to allow users to use the name that matches their installed gem name. They share the same internal objects and configuration settings.

How to test the change?

github-actions · 2025-01-02T23:50:21Z

Thank you for updating Change log entry section 👏

^{Visited at: 2025-01-09 11:52:07 UTC}

appraisal/generate.rb

datadog-datadog-prod-us1 · 2025-01-03T00:01:15Z

Datadog Report

Branch report: kicks
Commit report: 265d516
Test service: dd-trace-rb

✅ 0 Failed, 22124 Passed, 1477 Skipped, 5m 39.56s Total Time

…uns/12590455990

pr-commenter · 2025-01-03T00:09:43Z

Benchmarks

Benchmark execution time: 2025-01-08 19:59:51

Comparing candidate commit 265d516 in PR branch kicks with baseline commit e3ea38f in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

…uns/12643197954

appraisal/generate.rb

…uns/12656574953

…uns/12657714412

…uns/12661493382

codecov-commenter · 2025-01-08T00:41:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.73%. Comparing base (9440c03) to head (265d516).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4253      +/-   ##
==========================================
- Coverage   97.73%   97.73%   -0.01%     
==========================================
  Files        1352     1352              
  Lines       82409    82464      +55     
  Branches     4224     4229       +5     
==========================================
+ Hits        80545    80595      +50     
- Misses       1864     1869       +5

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

…uns/12677575958

…uns/12677665641

cswatt · 2025-01-08T22:08:28Z

docs/GettingStarted.md

@@ -1112,6 +1112,32 @@ end
 | --------- | ------------------------------- | ------ | -------------------------------------------- | ------- |
 | `enabled` | `DD_TRACE_KAFKA_ENABLED` | `Bool` | Whether the integration should create spans. | `true` |

+### Kicks
+
+The Kicks integration is a server-side middleware which will trace job executions.


Suggested change

The Kicks integration is a server-side middleware which will trace job executions.

The Kicks integration is a server-side middleware that traces job executions.

(I noticed most of these were copy-pasted from the sneakers docs, so we should probably propagate the improvements to both)

cswatt · 2025-01-08T22:10:07Z

docs/GettingStarted.md

+The Kicks integration is a server-side middleware which will trace job executions.
+
+<div class="alert alert-warning">
+Kicks is a continuation of Sneakers and both cannot be active at the same time, as they share their Ruby class namespace. Configurations to the Sneakers integration will be safely merged with any Kicks configuration.


Suggested change

Kicks is a continuation of Sneakers and both cannot be active at the same time, as they share their Ruby class namespace. Configurations to the Sneakers integration will be safely merged with any Kicks configuration.

Kicks is a continuation of Sneakers. Because Kicks and Sneakers share a Ruby class namespace, they cannot both be active at the same time. Configurations to the Sneakers integration is safely merged with any Kicks configuration.

cswatt · 2025-01-08T22:10:58Z

docs/GettingStarted.md

+| ---------- | - | ----- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- |
+| `enabled`  | `DD_TRACE_SNEAKERS_ENABLED` | `Bool` | Whether the integration should create spans.                                                                      | `true`                                                            |
+| `tag_body` | | `Bool` | Enable tagging of job message. `true` for on, `false` for off.                                                                                                              | `false`                                                           |
+| `on_error` | | `Proc` | Custom error handler invoked when a job raises an error. Provided `span` and `error` as arguments. Sets error on the span by default. Useful for ignoring transient errors. | `proc { \|span, error\| span.set_error(error) unless span.nil? }` |


Suggested change

| `on_error` | | `Proc` | Custom error handler invoked when a job raises an error. Provided `span` and `error` as arguments. Sets error on the span by default. Useful for ignoring transient errors. | `proc { \|span, error\| span.set_error(error) unless span.nil? }` |

| `on_error` | | `Proc` | Custom error handler invoked when a job raises an error. Provides `span` and `error` as arguments. Sets error on the span by default. Useful for ignoring transient errors. | `proc { \|span, error\| span.set_error(error) unless span.nil? }` |

cswatt · 2025-01-08T22:11:20Z

docs/GettingStarted.md

@@ -1935,6 +1961,10 @@ end

 The Sneakers integration is a server-side middleware which will trace job executions.

+<div class="alert alert-warning">
+Kicks is a continuation of Sneakers and both cannot be active at the same time, as they share their Ruby class namespace. Configurations to the Sneakers integration will be safely merged with any Kicks configuration.


Suggested change

Kicks is a continuation of Sneakers and both cannot be active at the same time, as they share their Ruby class namespace. Configurations to the Sneakers integration will be safely merged with any Kicks configuration.

Kicks is a continuation of Sneakers. Because Kicks and Sneakers share a Ruby class namespace, they cannot both be active at the same time. Configurations to the Sneakers integration is safely merged with any Kicks configuration.

ivoanjo

👍 LGTM Looks really nice :D

ivoanjo · 2025-01-09T09:42:53Z

sig/datadog/tracing/contrib/registerable.rbs

I actually noticed that this file was still being ignored in the Steepfile, but can safely be enabled (passes typecheck), so may be worth including that in this PR ;)

ivoanjo · 2025-01-09T09:54:22Z

lib/datadog/tracing/contrib/sneakers/integration.rb

          # @public_api Changing the integration name or integration options can cause breaking changes
          register_as :sneakers, auto_patch: true
-
+          register_as_alias :sneakers, :kicks


I noticed that for register_as_alias the auto_patch is always false -- is that because it reuses the auto_patch from the register_as? (If so, maybe that's a useful comment to leave somewhere, perhaps in the register_as_alias?)

ivoanjo · 2025-01-09T09:59:12Z

docs/GettingStarted.md

@@ -1112,6 +1112,32 @@ end
 | --------- | ------------------------------- | ------ | -------------------------------------------- | ------- |
 | `enabled` | `DD_TRACE_KAFKA_ENABLED` | `Bool` | Whether the integration should create spans. | `true` |

+### Kicks
+
+The Kicks integration is a server-side middleware which will trace job executions.


(I noticed most of these were copy-pasted from the sneakers docs, so we should probably propagate the improvements to both)

ivoanjo · 2025-01-09T10:05:56Z

appraisal/ruby-3.3.rb

@@ -131,7 +133,6 @@
  gem 'roda', '>= 2.0.0'
  gem 'semantic_logger', '~> 4.0'
  gem 'sidekiq', '~> 7'
-  gem 'sneakers', '>= 2.12.0'


Should this change be propagated to the other appraisal files as well? Noticed the others still list sneakers under contrib

TonyCTHsu

I understand this approach to support kicks.

However, I am a little bit concern about

kicks gem decided to rename the modules and paths in the future
The span data is decorated with the obsolete name sneakers can cause confusion

I am overall supportive because (1) might not happen in the future and (2) we could make the risk explicit in the documentation.

I might even push to deprecate sneakers integration and replace it completely with kicks in the next major release.

TonyCTHsu · 2025-01-09T11:56:54Z

appraisal/ruby-3.3.rb

@@ -131,7 +133,6 @@
  gem 'roda', '>= 2.0.0'
  gem 'semantic_logger', '~> 4.0'
  gem 'sidekiq', '~> 7'
-  gem 'sneakers', '>= 2.12.0'


@marcotc Do you struggle to remove sneakers from other runtimes? Do you need my help on this?

TonyCTHsu · 2025-01-09T12:04:04Z

lib/datadog/tracing/contrib/sneakers/integration.rb

+          # The last version of Sneakers is 2.12.0.
+          # The first version of Kicks is 3.0.0. We currently support all versions of Kicks.
+          #
+          # @see https://github.com/jondot/sneakers/commit/9780692624c666b6db8266d2d5710f709cb0f2e2
          def self.version


They kept everything the same, even the file paths (require 'sneakers') and module names (::Sneakers).

kicks even continue the semantic version to avoid conflict with the obsolete gem.

Correct me if I am wrong. Due to this, I assume the easiest path to support kicks is to change self.version with the conditional logic of kicks from Gem.loaded_specs. Do we need register_as_alias to support kicks?

TonyCTHsu · 2025-01-09T12:26:22Z

appraisal/jruby-9.2.rb

@@ -202,6 +202,8 @@
 build_coverage_matrix('stripe', 7..12, min: '5.15.0')
 build_coverage_matrix('opensearch', 2..3, gem: 'opensearch-ruby')
 build_coverage_matrix('elasticsearch', 7..8)
+build_coverage_matrix('kicks', [], min: '3.0.0')
+build_coverage_matrix('sneakers', [], min: '2.12.0', latest: false) # Sneakers is not receiving updates anymore and 2.12.0 is the last version


I actually think we don't need to invoke build_coverage_matrix with latest: false, since this method call only generate a single group.

Prefer the following for explicitness

appraisal 'sneakers-min' do gem 'sneakers', '= 2.12.0' end

TonyCTHsu · 2025-01-09T12:29:02Z

appraisal/generate.rb

+# @param [Hash] meta optional, additional metadata (development dependencies, etc.) for the group
+def build_coverage_matrix(integration, range, gem: integration, min: nil, latest: true, meta: {})
+  # Allow single version to be passed easily
+  range = range..range if range.is_a?(Integer)


Suggested change

range = range..range if range.is_a?(Integer)

range = Array(range)

ivoanjo · 2025-01-09T21:06:57Z

kicks gem decided to rename the modules and paths in the future

I'm guessing they would rename on a major version, so we'd probably want to support the 3.x series (with sneakers naming) for a while?

Looking at their readme, it seems the main objective was "original author was unresponsive, community had a few pending patches":

...so yeah doesn't look like the kind of project that will refactor just for the sake of making the name prettier ;)

github-actions bot added integrations Involves tracing integrations tracing labels Jan 2, 2025

datadog-datadog-prod-us1 bot reviewed Jan 2, 2025

View reviewed changes

appraisal/generate.rb Outdated Show resolved Hide resolved

Add support for Kicks gem

1396121

marcotc force-pushed the kicks branch from 1e42f87 to 1396121 Compare January 3, 2025 00:00

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

7bd5b43

…uns/12590455990

marcotc and others added 4 commits January 3, 2025 16:11

Update tests for newer version

0643968

Update minimum version

1d273cf

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

a4c0ca6

…uns/12643197954

Fix min version

9351e39

datadog-datadog-prod-us1 bot reviewed Jan 7, 2025

View reviewed changes

appraisal/generate.rb Show resolved Hide resolved

marcotc and others added 5 commits January 7, 2025 17:57

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

03db6f2

…uns/12656574953

Remove from contrib

c5c1384

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

8cf6a59

…uns/12657714412

Merge branch 'master' into kicks

efd850e

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

b018621

…uns/12661493382

marcotc and others added 3 commits January 8, 2025 11:22

Merge branch 'master' into kicks

6d6743d

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

539da60

…uns/12677575958

[🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/r…

265d516

…uns/12677665641

marcotc marked this pull request as ready for review January 8, 2025 21:45

marcotc requested review from a team as code owners January 8, 2025 21:45

cswatt reviewed Jan 8, 2025

View reviewed changes

ivoanjo approved these changes Jan 9, 2025

View reviewed changes

TonyCTHsu approved these changes Jan 9, 2025

View reviewed changes

TonyCTHsu reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for the `kicks` gem #4253

Add support for the `kicks` gem #4253

marcotc commented Jan 2, 2025 •

edited by Strech

Loading

github-actions bot commented Jan 2, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 3, 2025 •

edited

Loading

pr-commenter bot commented Jan 3, 2025 •

edited

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

cswatt Jan 8, 2025

ivoanjo Jan 9, 2025

cswatt Jan 8, 2025

cswatt Jan 8, 2025

cswatt Jan 8, 2025

ivoanjo left a comment

ivoanjo Jan 9, 2025

ivoanjo Jan 9, 2025

ivoanjo Jan 9, 2025

ivoanjo Jan 9, 2025

TonyCTHsu left a comment

TonyCTHsu Jan 9, 2025

TonyCTHsu Jan 9, 2025 •

edited

Loading

TonyCTHsu Jan 9, 2025

TonyCTHsu Jan 9, 2025

ivoanjo commented Jan 9, 2025

	The Kicks integration is a server-side middleware which will trace job executions.
	The Kicks integration is a server-side middleware that traces job executions.

	Kicks is a continuation of Sneakers and both cannot be active at the same time, as they share their Ruby class namespace. Configurations to the Sneakers integration will be safely merged with any Kicks configuration.
	Kicks is a continuation of Sneakers. Because Kicks and Sneakers share a Ruby class namespace, they cannot both be active at the same time. Configurations to the Sneakers integration is safely merged with any Kicks configuration.

	\| `on_error` \| \| `Proc` \| Custom error handler invoked when a job raises an error. Provided `span` and `error` as arguments. Sets error on the span by default. Useful for ignoring transient errors. \| `proc { \\|span, error\\| span.set_error(error) unless span.nil? }` \|
	\| `on_error` \| \| `Proc` \| Custom error handler invoked when a job raises an error. Provides `span` and `error` as arguments. Sets error on the span by default. Useful for ignoring transient errors. \| `proc { \\|span, error\\| span.set_error(error) unless span.nil? }` \|

	range = range..range if range.is_a?(Integer)
	range = Array(range)

Add support for the kicks gem #4253

Are you sure you want to change the base?

Add support for the kicks gem #4253

Conversation

marcotc commented Jan 2, 2025 • edited by Strech Loading

github-actions bot commented Jan 2, 2025 • edited Loading

datadog-datadog-prod-us1 bot commented Jan 3, 2025 • edited Loading

Datadog Report

pr-commenter bot commented Jan 3, 2025 • edited Loading

Benchmarks

codecov-commenter commented Jan 8, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonyCTHsu Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivoanjo commented Jan 9, 2025

Add support for the `kicks` gem #4253

Add support for the `kicks` gem #4253

marcotc commented Jan 2, 2025 •

edited by Strech

Loading

github-actions bot commented Jan 2, 2025 •

edited

Loading

datadog-datadog-prod-us1 bot commented Jan 3, 2025 •

edited

Loading

pr-commenter bot commented Jan 3, 2025 •

edited

Loading

codecov-commenter commented Jan 8, 2025 •

edited

Loading

TonyCTHsu Jan 9, 2025 •

edited

Loading