chore: Replace positive? with comparison operator

[miry]

Opentelemetry is utilized across numerous projects, and even minor performance variations can have a significant impact.
It's worth noting that the default rubocop style may not have been specifically crafted with performance considerations in mind.

According to benchmarks on my machine M1, it appears that Numeric#positive? is approximately 1.32 times slower than using the comparison operator (number > 0).

benchmark script

ruby 3.3.0 (2023-12-25 revision 5124f9ac75) +YJIT [arm64-darwin23]
Warming up --------------------------------------
      compare with 0     1.000 i/100ms
           positive?     1.000 i/100ms
Calculating -------------------------------------
      compare with 0      3.153 (± 0.0%) i/s -     95.000 in  30.132600s
           positive?      2.397 (± 0.0%) i/s -     72.000 in  30.042688s

Comparison:
      compare with 0:        3.2 i/s
           positive?:        2.4 i/s - 1.32x  slower

The change disables Style/NumericPredicate for rubocop as well for sdk.

Propose to change for sdk gem in BatchSpanProcessor.
The number suppose not be a nil or BigNumber.

References

• positive? vs >0 cops rubocop/rubocop#5564 (comment)
• Disable the NumericPredicate cop autolist/autocop#33
• https://rubyapi.org/3.3/o/numeric#method-i-positive-3F

[kaylareopelle]

Thank you for this catch! We use rubocop-performance in the contrib repo, but haven't implemented it here.

It looks like this particular fix might not be part of rubocop-performance yet (cops list), so you could consider contributing a new cop with this swap to that repo as well! 😄

[miry]

@kaylareopelle Thank you for the hint.

[miry]

@kaylareopelle The problem with this cop is that it is hard to know the type of variable. The change is good when I know for sure there is a Number and not another object or nil. That's why I propose to disable Style cop instead of forcing comparision.

Anyway, I sent a PR to introduce a new cop: rubocop/rubocop-performance#440. I will wait for a response, but I think it may not be merged.

[robertlaurin]

Curious where you spotted this one, did it show up on a profile in a production load?

[miry]

Curious where you spotted this one, did it show up on a profile in a production load?

I lack access to high-load applications (not anymore).
I am unable to measure performance in a production environment.
Upon reviewing the code, I encountered a line that appears suspicious.
To investigate further, I conducted a small benchmark.

However, it would be beneficial to seek input from a Ruby group to validate my theory.