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

Add support for the kicks gem #4253

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Matrixfile
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@
'contrib' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby'
},
'sneakers' => {
'contrib' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby'
'kicks-latest' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby',
'kicks-min' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby',
'sneakers-min' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby',
},
'stripe' => {
'stripe-latest' => '✅ 2.5 / ✅ 2.6 / ✅ 2.7 / ✅ 3.0 / ✅ 3.1 / ✅ 3.2 / ✅ 3.3 / ✅ 3.4 / ✅ jruby',
Expand Down
29 changes: 17 additions & 12 deletions appraisal/generate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,15 @@

# Builds a matrix of versions to test for a given integration

# `range`: the range of versions to test
# `gem` : optional, gem name to test (gem name can be different from the integration name)
# `min` : optional, minimum version to test
# `meta` : optional, additional metadata (development dependencies, etc.) for the group
def build_coverage_matrix(integration, range, gem: nil, min: nil, meta: {})
gem ||= integration
# @param [String] integration the name of the integration to test
# @param [Range, Integer] range the range of major versions to test, or a single major version to test
# @param [String] gem optional, gem name to test (gem name can be different from the integration name)
# @param [String] min optional, minimum version to test
# @param [Boolean] latest optional, whether to test the latest version
# @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: {})
marcotc marked this conversation as resolved.
Show resolved Hide resolved
# Allow single version to be passed easily
range = range..range if range.is_a?(Integer)
Copy link
Contributor

Choose a reason for hiding this comment

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

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


if min
appraise "#{integration}-min" do
Expand All @@ -70,12 +73,14 @@ def build_coverage_matrix(integration, range, gem: nil, min: nil, meta: {})
end
end

appraise "#{integration}-latest" do
# The latest group declares dependencies without version constraints,
# still requires being updated to pick up the next major version and
# committing the changes to lockfiles.
gem gem
meta.each { |k, v| v ? gem(k, v) : gem(k) }
if latest
appraise "#{integration}-latest" do
# The latest group declares dependencies without version constraints,
# still requires being updated to pick up the next major version and
# committing the changes to lockfiles.
gem gem
meta.each { |k, v| v ? gem(k, v) : gem(k) }
end
end
end

Expand Down
2 changes: 2 additions & 0 deletions appraisal/jruby-9.2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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


appraise 'relational_db' do
gem 'activerecord', '~> 5'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/jruby-9.3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,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

appraise 'relational_db' do
gem 'activerecord', '~> 6.0.0'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/jruby-9.4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,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

appraise 'relational_db' do
gem 'activerecord', '~> 6.1.0'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-2.5.rb
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,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

appraise 'relational_db' do
gem 'activerecord', '~> 5'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-2.6.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,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

appraise 'relational_db' do
gem 'activerecord', '~> 6.0.0'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-2.7.rb
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,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

appraise 'relational_db' do
gem 'activerecord', '~> 6.1.0'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-3.0.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,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

appraise 'relational_db' do
gem 'activerecord', '~> 7'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-3.1.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,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

appraise 'relational_db' do
gem 'activerecord', '~> 7'
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-3.2.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,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

appraise 'relational_db' do
gem 'activerecord', '~> 7'
Expand Down
3 changes: 2 additions & 1 deletion appraisal/ruby-3.3.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,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

appraise 'relational_db' do
gem 'activerecord', '~> 7'
Expand Down Expand Up @@ -131,7 +133,6 @@
gem 'roda', '>= 2.0.0'
gem 'semantic_logger', '~> 4.0'
gem 'sidekiq', '~> 7'
gem 'sneakers', '>= 2.12.0'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

gem 'sucker_punch'
gem 'que', '>= 1.0.0'
end
Expand Down
2 changes: 2 additions & 0 deletions appraisal/ruby-3.4.rb
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,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

appraise 'relational_db' do
# ActiveRecord locked because tests are failing with 7.1, which was attempted as a part of Ruby 3.4 testing in CI.
Expand Down
3 changes: 3 additions & 0 deletions docs/Compatibility.md
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ For a list of available integrations, and their configuration options, refer to
| httpclient | `httpclient` | `>= 2.2` | `>= 2.2` | [Link][23] | [Link](https://github.com/nahi/httpclient) |
| httpx | `httpx` | `>= 0.11` | `>= 0.11` | [Link][24] | [Link](https://gitlab.com/honeyryderchuck/httpx) |
| Kafka | `ruby-kafka` | `>= 0.7.10` | `>= 0.7.10` | [Link][25] | [Link](https://github.com/zendesk/ruby-kafka) |
| Kicks | `kicks` | `>= 3.0.0` | `>= 3.0.0` | [Link][55] | [Link](https://github.com/ruby-amqp/kicks) |
| Makara (via Active Record) | `makara` | `>= 0.3.5` | `>= 0.3.5` | [Link][8] | [Link](https://github.com/instacart/makara) |
| MongoDB | `mongo` | `>= 2.1` | `>= 2.1` | [Link][26] | [Link](https://github.com/mongodb/mongo-ruby-driver) |
| MySQL2 | `mysql2` | `>= 0.3.21` | *gem not available* | [Link][27] | [Link](https://github.com/brianmario/mysql2) |
Expand Down Expand Up @@ -268,3 +269,5 @@ new release of 1.21.0 (or 1.20.1). Those bugfixes will not be backported as patc
[53]: https://docs.datadoghq.com/agent/basic_agent_usage/?tab=agentv6v7

[54]: https://docs.datadoghq.com/agent/basic_agent_usage/?tab=agentv5

[55]: https://docs.datadoghq.com/tracing/trace_collection/dd_libraries/ruby#kicks
30 changes: 30 additions & 0 deletions docs/GettingStarted.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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


<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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</div>

You can enable it through `Datadog.configure`:

```ruby
require 'datadog'

Datadog.configure do |c|
c.tracing.instrument :kicks, **options
end
```

`options` are the following keyword arguments:

| Key | Env Var | Type | Description | Default |
| ---------- | - | ----- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------- |
| `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? }` |
Copy link
Contributor

Choose a reason for hiding this comment

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

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? }` |


### MongoDB

The integration traces any `Command` that is sent from the [MongoDB Ruby Driver](https://github.com/mongodb/mongo-ruby-driver) to a MongoDB cluster. By extension, Object Document Mappers (ODM) such as Mongoid are automatically instrumented if they use the official Ruby driver. To activate the integration, simply:
Expand Down Expand Up @@ -1936,6 +1962,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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</div>

You can enable it through `Datadog.configure`:

```ruby
Expand Down
1 change: 0 additions & 1 deletion gemfiles/ruby_3.3_contrib.gemfile

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 0 additions & 20 deletions gemfiles/ruby_3.3_contrib.gemfile.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions lib/datadog/tracing/contrib/registerable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,17 @@ module ClassMethods
def register_as(name, registry: Contrib::REGISTRY, auto_patch: false, **options)
registry.add(name, new(name, **options), auto_patch)
end

# Registers this `alias_name` in the global tracer registry as an alias of `original_name`.
# Using `alias_name` or `original_name` become interchangeable.
# The configuration object is shared between the two names.
# The patcher will only run once if both are activated.
def register_as_alias(original_name, alias_name, registry: Contrib::REGISTRY)
original = registry[original_name]
raise ArgumentError, "integration '#{original_name}' not registered" unless original

registry.add(alias_name, original, false)
end
end

# Instance methods for registerable behavior
Expand Down
20 changes: 16 additions & 4 deletions lib/datadog/tracing/contrib/sneakers/integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,21 +13,33 @@ module Sneakers
class Integration
include Contrib::Integration

MINIMUM_VERSION = Gem::Version.new('2.12.0')
MINIMUM_SNEAKERS_VERSION = Gem::Version.new('2.12.0')
# All versions are supported. Kicks first version is 3.0.0.
MINIMUM_KICKS_VERSION = Gem::Version.new('3.0.0')

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

register_as_alias :sneakers, :kicks
Comment on lines 20 to +22
Copy link
Member

Choose a reason for hiding this comment

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

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


# Sneakers development continues in the Kicks gem.
# The **only** thing that has changed is the gem name,
# even the file naming and module namespacing are the same (require 'sneakers', `::Sneakers`).
#
# 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
Copy link
Contributor

@TonyCTHsu TonyCTHsu Jan 9, 2025

Choose a reason for hiding this comment

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

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?

Gem.loaded_specs['sneakers'] && Gem.loaded_specs['sneakers'].version
Gem.loaded_specs['sneakers'] && Gem.loaded_specs['sneakers'].version || \
Gem.loaded_specs['kicks'] && Gem.loaded_specs['kicks'].version
end

def self.loaded?
!defined?(::Sneakers).nil?
end

def self.compatible?
super && version >= MINIMUM_VERSION
super && version >= MINIMUM_SNEAKERS_VERSION
end

def new_configuration
Expand Down
2 changes: 2 additions & 0 deletions sig/datadog/tracing/contrib/registerable.rbs
Copy link
Member

Choose a reason for hiding this comment

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

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

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ module Datadog
def self.included: (untyped base) -> untyped
module ClassMethods
def register_as: (untyped name, ?registry: untyped, ?auto_patch: bool, **untyped options) -> untyped

def register_as_alias: (Symbol original_name, Symbol alias_name, registry: Registry) -> void
end
module InstanceMethods
attr_reader name: untyped
Expand Down
Loading
Loading