Skip to content

Streamline ddsketch loading, remove circular dependencies #5008

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Streamline ddsketch loading, remove circular dependencies #5008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
971e1f4
Revert "Change the requires such that there is no circular dependency…
p Oct 29, 2025
2534ffd
move require to top level
p Oct 29, 2025
a2a3d0c
move ddsketch supported method up to Core module, remove requires of …
p Oct 29, 2025
0eb70cd
remove more ddsketch requires
p Oct 29, 2025
57a860a
remove test case
p Oct 29, 2025
0d84417
type
p Oct 29, 2025
c293757
Update lib/datadog/core.rb
p-datadog Nov 7, 2025
78bce97
type
p Nov 7, 2025
399763c
Merge branch 'master' into core-libdatadog-dep
ivoanjo Nov 24, 2025
60046cc
Merge branch 'master' into core-libdatadog-dep
p Nov 26, 2025
e474def
put in class stub as per profiling
p Nov 26, 2025
4935cc3
add api private
p Nov 26, 2025
d6de109
use description from before
p Nov 26, 2025
ecf4b36
we need to fail the constructor then
p Nov 26, 2025
9fc0113
cannot define the class without problems, replace it with a comment
p Nov 26, 2025
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: 4 additions & 0 deletions lib/datadog/core.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,10 @@ module Core
rescue LoadError => e
e.message
end

def self.ddsketch_supported?
LIBDATADOG_API_FAILURE.nil?
end
end

DATADOG_ENV = Core::Configuration::ConfigHelper.new
Expand Down
24 changes: 11 additions & 13 deletions lib/datadog/core/ddsketch.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,16 @@

module Datadog
module Core
# Used to access ddsketch APIs.
# APIs in this class are implemented as native code.
class DDSketch
def self.supported?
Datadog::Core::LIBDATADOG_API_FAILURE.nil?
end

def initialize
unless self.class.supported?
raise(ArgumentError, "DDSketch is not supported: #{Datadog::Core::LIBDATADOG_API_FAILURE}")
end
end
end
# Datadog::Core::DDSketch is defined completely in the native extension.
# Do not define it here (for example, as an empty class) because we don't
# want to be able to create instances of the empty stub class if the
# native extension is missing or failed to load.
Comment on lines +5 to +8
Copy link
Member

@ivoanjo ivoanjo Nov 26, 2025

Choose a reason for hiding this comment

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

It's not a blocker for this PR if you don't agree with me, but I'm not a fan of this pattern of working really hard not to define the class.

In particular, because I've seen that in profiling, even when classes start as pure-native, they often evolve small Ruby-level helpers that are 200% easier to write in Ruby code instead of writing some gnarly C code "just to avoid adding a Ruby file".

So I think living with the empty definition here is better than just keeping a comment; also because it lets tools such as LSPs know where a class is defined (and then you can read the comment attached to the class that tells you "the rest is in C").

What profiling does in exchange is that the profiling classes are only required if the native extension is loaded successfully and I would suggest adopting the pattern here. Specifically: define the class in this file but make it so it won't exist if the extension isn't loaded. This happens not because the class is not defined in Ruby code but because the Ruby code doesn't get required unless the extension is up and running.

Copy link
Member

@Strech Strech Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to add that core.rb now has knowledge of ddsketch when maybe should not due to its nature of being "general" class/module.

#
# Use Core.ddsketch_supported? to determine if DDSketch class exists and
# is usable.
#
# See https://github.com/datadog/dd-trace-rb/pull/5008 and
# https://github.com/DataDog/dd-trace-rb/pull/4901 for the background on
# dependency issues with DDSketch.
end
end
3 changes: 1 addition & 2 deletions lib/datadog/data_streams/processor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
require_relative '../version'
require_relative '../core/worker'
require_relative '../core/workers/polling'
require_relative '../core/ddsketch'
require_relative '../core/buffer/cruby'
require_relative '../core/utils/time'

Expand Down Expand Up @@ -39,7 +38,7 @@ class Processor < Core::Worker
# (default: DEFAULT_BUFFER_SIZE). Higher values support more throughput but use more memory.
# @raise [UnsupportedError] if DDSketch is not available on this platform
def initialize(interval:, logger:, settings:, agent_settings:, buffer_size: DEFAULT_BUFFER_SIZE)
raise UnsupportedError, 'DDSketch is not supported' unless Datadog::Core::DDSketch.supported?
raise UnsupportedError, 'DDSketch is not supported' unless Datadog::Core.ddsketch_supported?

@settings = settings
@agent_settings = agent_settings
Expand Down
2 changes: 2 additions & 0 deletions sig/datadog/core.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@ module Datadog
module Core
LIBDATADOG_API_FAILURE: ::String?
extend Core::Deprecations

def self.ddsketch_supported?: () -> bool
end
end
2 changes: 0 additions & 2 deletions sig/datadog/core/ddsketch.rbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
module Datadog
module Core
class DDSketch
def self.supported?: () -> bool

# Adds a single point to the sketch
# @param point [::Numeric] The value to add to the sketch
# @return [true] Always returns true on success, raises RuntimeError on failure
Expand Down
12 changes: 0 additions & 12 deletions spec/datadog/core/ddsketch_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,6 @@
require 'datadog/core'
require 'datadog/core/ddsketch'
require 'datadog/core/ddsketch_pprof/ddsketch_pb'

RSpec.describe Datadog::Core::DDSketch do
context 'when DDSketch is not supported' do
before do
stub_const('Datadog::Core::LIBDATADOG_API_FAILURE', 'Example error loading libdatadog_api')
end

it 'raises an error' do
expect { described_class.new }.to raise_error(ArgumentError, 'DDSketch is not supported: Example error loading libdatadog_api')
end
end

context 'when DDSketch is supported' do
subject(:sketch) { described_class.new }

Expand Down
2 changes: 0 additions & 2 deletions spec/datadog/data_streams/processor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# frozen_string_literal: true

require 'datadog/core'
require 'datadog/data_streams/processor'
require 'datadog/core/ddsketch'
require_relative 'spec_helper'

# Expected deterministic hash values for specific pathways (with manual_checkpoint: false)
Expand Down
2 changes: 1 addition & 1 deletion spec/datadog/data_streams/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def skip_if_data_streams_not_supported(testcase)
)
end

return if Datadog::Core::DDSketch.supported?
return if Datadog::Core.ddsketch_supported?

# Ensure DDSketch was loaded correctly
raise "DDSketch does not seem to be available: #{Datadog::Core::LIBDATADOG_API_FAILURE}. " \
Expand Down
2 changes: 0 additions & 2 deletions spec/datadog/tracing/contrib/kafka/data_streams_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# frozen_string_literal: true

require 'datadog/tracing/contrib/support/spec_helper'
require 'datadog/core'
require 'datadog/core/ddsketch'
require 'datadog/data_streams/spec_helper'
require 'ostruct'
require 'datadog/tracing/contrib/kafka/integration'
Expand Down
2 changes: 0 additions & 2 deletions spec/datadog/tracing/contrib/karafka/data_streams_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
# frozen_string_literal: true

require 'datadog/tracing/contrib/support/spec_helper'
require 'datadog/core'
require 'datadog/core/ddsketch'
require 'datadog/data_streams/spec_helper'
require 'karafka'
require 'ostruct'
Expand Down