Skip to content

Conversation

@p-datadog
Copy link
Member

@p-datadog p-datadog commented Oct 29, 2025

What does this PR do?
Changes how ddsketch / libdatadog integration is handled:

Before, there was a lib/datadog/core/ddsketch.rb file which contained a class-level supported? method. Having this file meant that you could always require datadog/core/ddsketch, but you wouldn't always get a functional ddsketch out of that require.

The proposed change is to delete this file completely. Loading datadog/core will attempt to bring in ddsketch (and this is already behavior in master), therefore no additional require is needed for ddsketch.

To check for ddsketch presence, the supported? method was moved up to Datadog::Core module (up from Datadog::Core::DDSketch). Meaning:

  1. if ddsketch is not supported, Datadog::Core::DDSketch will be not defined at all.
  2. one does not need to require ddsketch to see if it's supported.

Motivation:
Removing circular dependency: ddsketch -> core -> configuration -> ddsketch

Change log entry
None

Additional Notes:

How to test the change?
Existing CI

@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Thank you for updating Change log entry section 👏

Visited at: 2025-11-06 13:31:16 UTC

@github-actions github-actions bot added core Involves Datadog core libraries integrations Involves tracing integrations tracing labels Oct 29, 2025
@github-actions
Copy link

github-actions bot commented Oct 29, 2025

Typing analysis

This PR does not change typing compared to the base branch.

@pr-commenter
Copy link

pr-commenter bot commented Oct 29, 2025

Benchmarks

Benchmark execution time: 2025-11-26 01:51:34

Comparing candidate commit 9fc0113 in PR branch core-libdatadog-dep with baseline commit f045f9d in branch master.

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

scenario:tracing - Propagation - Trace Context

  • 🟩 throughput [+2925.324op/s; +3031.308op/s] or [+8.544%; +8.854%]

@p-datadog p-datadog force-pushed the core-libdatadog-dep branch from b17d209 to 0d84417 Compare November 5, 2025 21:48
@datadog-official
Copy link

datadog-official bot commented Nov 5, 2025

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

🎯 Code Coverage
Patch Coverage: 100.00%
Total Coverage: 95.15% (-0.02%)

View detailed report

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 9fc0113 | Docs | Datadog PR Page | Was this helpful? Give us feedback!

@p-datadog p-datadog marked this pull request as ready for review November 6, 2025 13:36
@p-datadog p-datadog requested review from a team as code owners November 6, 2025 13:36
@p-datadog p-datadog requested a review from vpellan November 6, 2025 13:36
@p-datadog
Copy link
Member Author

CI is failing due to missing type declaration, I'll add it if there is consensus that this change is the way to go.

ivoanjo
ivoanjo previously approved these changes Nov 6, 2025
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

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

👍 So I think this change is fine, but I'm curious -- where is the circular dependency again?

You listed

Removing circular dependency: ddsketch -> core -> configuration -> ddsketch

But... lib/datadog/core/ddsketch.rb did not require anything before, so I'm not sure where the circular dependency again? I played with the master branch before a bit and I could not see any circular require warnings.

@p-datadog
Copy link
Member Author

The technical circular dependency was removed in 43e1f23#diff-6566ed78aec0d0209e0f3b7776e7db57d4694f438d12d3b6b9312f1209bfd994L3, specifically the require of 'datadog/core' from 'datadog/core/ddsketch'. However, 'datadog/core/ddsketch' still references Datadog::Core, but now without the require being present.

So, technically, the circular dependency is there, but not in 'require' form at the moment.

@ivoanjo
Copy link
Member

ivoanjo commented Nov 7, 2025

Oh, right, I understand. Indeed Datadog::Core::DDSketch.supported? would break unless core was loaded.

@p-datadog p-datadog requested a review from ericfirth November 7, 2025 13:12
ivoanjo and others added 2 commits November 24, 2025 15:14
* master:
  [PROF-13115] Bootstrap installing dependencies on Ruby 4.0.0-preview2
  Clarify support for `rb_obj_info` and why it's OK to not have it
  Tweak pending to not apply to all Ruby preview versions
  Do not try to use `rb_obj_info` on Ruby 4.0
  Adjust stack collector spec to account for changed Ruby 4 behavior
  [PROF-13115] Disable heap profiling on Ruby 4 preview due to incompatibility
  Rewrite security response tests
@p-datadog
Copy link
Member Author

I tried to put back in the empty class but this causes issues because it's not supposed to be creatable when the C extension is missing, and trying to fix that took me back to the current state of things before the changes in this PR.

I put in the ddsketch.rb so that type checking rake task is happy, and that file only contains a comment about the DDSketch class, but no definition of the class itself.

Comment on lines +5 to +8
# 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.
Copy link
Member

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

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.

@p-datadog
Copy link
Member Author

CI default configuration does not have libdatadog_api extension, check into using that for the test that ddsketch is missing

@ivoanjo ivoanjo dismissed their stale review November 27, 2025 09:01

(Dismissing since my approval was based on older versions, and the approach has changed since. As noted in my comment -- my feedback is not blocking either, there's an existing approval already)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Involves Datadog core libraries integrations Involves tracing integrations tracing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants