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

[NO-TICKET] RFC: Add logger gem as dependency to prepare for future Ruby versions #4257

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

ivoanjo
Copy link
Member

@ivoanjo ivoanjo commented Jan 6, 2025

What does this PR do?

This PR declares a dependency from the datadog gem to the logger gem.

While every PR is an RFC, I explicitly marked this one as such since adding a new dependency should never be done lightly.

Motivation:

This is needed to avoid a warning:

lib/datadog/core/configuration/settings.rb:3: warning: logger was
loaded from the standard library, but will no longer be part of the
default gems starting from Ruby 3.5.0.
You can add logger to your Gemfile or gemspec to silence this warning.

Change log entry

Yes. Add logger gem as dependency to prepare for future Ruby versions

Additional Notes:

logger is just the latest in a long line of gems that have been refactored out in this way. See
ruby/ruby@d7e558e and https://stdgems.org/ (not yet updated for 3.5 as of this writing).

While in the past we've avoided adding extra dependencies due to this extraction work, I think this one does make sense to add:

  1. It's supported on every Ruby version we currently support (>= 2.5)
  2. It's not a small amount of code to shed/rewrite. And on the other side, I don't see a strong reason for embedding instead of depending on it.

We're getting warnings for two more gems: ostruct and benchmark.

From our internal discussions, we can probably get rid of our usage of them, rather than promote them to a dependency, so that's why I did not touch those warnings in this PR.

How to test the change?

The following script can be used (on Ruby 3.4 and above) to trigger the warning, and to confirm it's gone once logger is added as a dependency:

require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'datadog', path: '.'
end

puts RUBY_DESCRIPTION
require 'datadog'
puts Datadog::VERSION::STRING

Make sure to run with just ruby example.rb and not with bundle exec.

… Ruby versions

**What does this PR do?**

This PR declares a dependency from the `datadog` gem to the `logger`
gem.

While every PR is an RFC, I explicitly marked this one as such since
adding a new dependency should never be done lightly.

**Motivation:**

This is needed to avoid a warning:

> lib/datadog/core/configuration/settings.rb:3: warning: logger was
> loaded from the standard library, but will no longer be part of the
> default gems starting from Ruby 3.5.0.
> You can add logger to your Gemfile or gemspec to silence this warning.

**Additional Notes:**

`logger` is just the latest in a long line of gems that have been
refactored out in this way. See
ruby/ruby@d7e558e
and https://stdgems.org/ (not yet updated for 3.5 as of this writing).

While in the past we've avoided adding extra dependencies due to this
extraction work, I think this one does make sense to add:

1. It's supported on every Ruby version we currently support (>= 2.5)
2. It's not a small amount of code to shed/rewrite. And on the other
   side, I don't see a strong reason for embedding instead of depending
   on it.

We're getting warnings for two more gems: `ostruct` and `benchmark`.

From our internal discussions, we can probably get rid of our usage
of them, rather than promote them to a dependency, so that's why I
did not touch those warnings in this PR.

**How to test the change?**

The following script can be used (on Ruby 3.4 and above) to trigger
the warning, and to confirm it's gone once `logger` is added as
a dependency:

```ruby
require 'bundler/inline'

gemfile do
  source 'https://rubygems.org'

  gem 'datadog', path: '.'
end

puts RUBY_DESCRIPTION
require 'datadog'
puts Datadog::VERSION::STRING
```

Make sure to run with just `ruby example.rb` and not with
`bundle exec`.
@ivoanjo ivoanjo requested review from a team as code owners January 6, 2025 16:21
@ivoanjo ivoanjo added the core Involves Datadog core libraries label Jan 6, 2025
@github-actions github-actions bot added the single-step Single Step APM Instrumentation label Jan 6, 2025
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Jan 6, 2025

Datadog Report

Branch report: ivoanjo/add-dependency-on-logger-gem
Commit report: 714e329
Test service: dd-trace-rb

✅ 0 Failed, 22051 Passed, 1477 Skipped, 5m 19.48s Total Time

@codecov-commenter
Copy link

codecov-commenter commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.71%. Comparing base (7e37aff) to head (714e329).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4257   +/-   ##
=======================================
  Coverage   97.70%   97.71%           
=======================================
  Files        1358     1358           
  Lines       82502    82502           
  Branches     4223     4223           
=======================================
+ Hits        80610    80618    +8     
+ Misses       1892     1884    -8     

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

@ivoanjo
Copy link
Member Author

ivoanjo commented Jan 8, 2025

While I don't expect this to have any fallback, changing dependencies always makes me a bit nervous so I'll mark this as do-not-merge until we put the 2.9.0 release out.

This way we can merge it in early during the 2.10 cycle and have a bit longer baking time.

@ivoanjo ivoanjo added the do-not-merge/WIP Not ready for merge label Jan 8, 2025
@pr-commenter
Copy link

pr-commenter bot commented Jan 17, 2025

Benchmarks

Benchmark execution time: 2025-01-17 10:11:57

Comparing candidate commit 714e329 in PR branch ivoanjo/add-dependency-on-logger-gem with baseline commit 7e37aff in branch master.

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

scenario:profiler - hold / resume

  • 🟩 throughput [+81375.926op/s; +82728.413op/s] or [+5.002%; +5.085%]

scenario:tracing - Propagation - Trace Context

  • 🟩 throughput [+3329.938op/s; +3423.551op/s] or [+9.697%; +9.970%]

@ivoanjo
Copy link
Member Author

ivoanjo commented Jan 17, 2025

Now that 2.9 is out, I'm going to go ahead and merge this one. Thanks for the help, folks!

@ivoanjo ivoanjo removed the do-not-merge/WIP Not ready for merge label Jan 17, 2025
@ivoanjo ivoanjo merged commit 41dc832 into master Jan 17, 2025
388 checks passed
@ivoanjo ivoanjo deleted the ivoanjo/add-dependency-on-logger-gem branch January 17, 2025 10:35
@github-actions github-actions bot added this to the 2.10.0 milestone Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries single-step Single Step APM Instrumentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants