Skip to content

Do not use benchmark gem in production - #324

Merged
Sinjo merged 2 commits into
prometheus:mainfrom
Juanmcuello:add-benchmark-to-gemspec
Jun 21, 2025
Merged

Do not use benchmark gem in production#324
Sinjo merged 2 commits into
prometheus:mainfrom
Juanmcuello:add-benchmark-to-gemspec

Conversation

@Juanmcuello

Copy link
Copy Markdown
Contributor

The benchmark gem is no longer a default gem in Ruby 3.5.

Add benchmark to the gemspec, for forward compatibility with Ruby 3.5 and to resolve the `benchmark was loaded from the standard library, but will no longer be part of the default gems starting from Ruby 3.5.0.' warning that Ruby 3.4 emits on loading client_ruby.

https://docs.ruby-lang.org/en/master/NEWS_md.html#label-Stdlib+updates

@Juanmcuello
Juanmcuello force-pushed the add-benchmark-to-gemspec branch from 1991367 to 1098458 Compare June 16, 2025 01:12
@dmagliola

Copy link
Copy Markdown
Collaborator

When I saw this I thought "surely this should be a development dependency, for our benchmarks, which almost no one will encounter", but we're actually using Benchmark in our Collector Middleware to measure response time...

I do wonder whether we should switch to the much more common Process.clock_gettime(Process::CLOCK_MONOTONIC) instead and lose this depenency.

@Sinjo @Juanmcuello any thoughts?

@Juanmcuello

Juanmcuello commented Jun 16, 2025

Copy link
Copy Markdown
Contributor Author

Yeah, I saw that too. I think dropping Benchmark would be better. There are some specs that depend on the lib, but I think everything could be replaced with something like this:

# lib/prometheus/middleware/collector.rb   
def realtime
  start_time = Process.clock_gettime(Process::CLOCK_MONOTONIC)
  yield
  Process.clock_gettime(Process::CLOCK_MONOTONIC) - start_time
end

# spec/prometheus/middleware/collector_spec.rb
expect(app).to receive(:realtime).and_yield.and_return(0.2)

I can try a PR if you think that's the way to go.

@Juanmcuello

Copy link
Copy Markdown
Contributor Author

But I see that even with these changes, we would still depend on Benchmark for testing, so we would have to keep it as a dev dependency anyway.

@dmagliola

Copy link
Copy Markdown
Collaborator

I can try a PR if you think that's the way to go.

I'd love @Sinjo 's thoughts, in case i'm missing something, but this seems better to me.

But I see that even with these changes, we would still depend on Benchmark for testing, so we would have to keep it as a dev dependency anyway.

Yes, but that I don't mind. Dev dependencies are not inflicted on everyone installing your gem (like real dependencies are), so I'm not concerned about that one.

@Sinjo

Sinjo commented Jun 18, 2025

Copy link
Copy Markdown
Member

Whoa, I had no idea we were using this in prod code! I'm +1 on making it a dev dependency and using Process.clock_gettime(Process::CLOCK_MONOTONIC) directly in prod code.

The benchmark gem is no longer a default gem in Ruby 3.5, so we've removed its
use by replacing the Benchmark.realtime method with our own implementation.

We still rely on the gem in our tests, though.

Signed-off-by: Juan Manuel Cuello <jcuello@fu.do>
It's no longer a default gem in Ruby 3.5, so we explicitly add it to the
gemspec.

Signed-off-by: Juan Manuel Cuello <jcuello@fu.do>
@Juanmcuello
Juanmcuello force-pushed the add-benchmark-to-gemspec branch from 1098458 to ddfae02 Compare June 19, 2025 00:25
@Juanmcuello Juanmcuello changed the title Declare benchmark gem dependency, ready for Ruby 3.5. Jun 19, 2025
@Juanmcuello

Copy link
Copy Markdown
Contributor Author

@Sinjo, great. I've updated the MR with the changes you mentioned, as well as the title. Let me know what you think.

@electron0zero electron0zero left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm :shipit:

@Sinjo
Sinjo merged commit dca6002 into prometheus:main Jun 21, 2025
@Sinjo

Sinjo commented Jun 21, 2025

Copy link
Copy Markdown
Member

Thank you @Juanmcuello! I'll get a release out this weekend.

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

Labels

None yet

4 participants