refactor: Freeze all range objects #1172

[Victorsesan]

Ref #1172

[arielvalentin]

@open-telemetry/ruby-contrib-approvers now that I'm looking at this I'm wondering if there is an opportunity to extract status setter helper method rather than defining a range per http instrumentation and implementing the same logic in each instrumentation separately.

Something like this

Semconv::Http::Client.set_status(span, http_status_code)

Which would then set the span status based on the http status code falling in the range of bad client status code.

That would reduce the number of places we would need to update when satisfying the specification.

Maybe that could be a follow up PR and this perf refactoring is a first step?

[kaylareopelle]

@open-telemetry/ruby-contrib-approvers now that I'm looking at this I'm wondering if there is an opportunity to extract status setter helper method rather than defining a range per http instrumentation and implementing the same logic in each instrumentation separately.

Something like this

Semconv::Http::Client.set_status(span, http_status_code)

Which would then set the span status based on the http status code falling in the range of bad client status code.

That would reduce the number of places we would need to update when satisfying the specification.

Maybe that could be a follow up PR and this perf refactoring is a first step?

@arielvalentin - I like that idea! My vote would be to keep this refactoring as a first step and open that up in a follow-up PR.

[kaylareopelle]

Hi @Victorsesan! Thanks for re-opening this PR!

It looks like there are a few rubocop (Ruby linter) failures on your changes.

The general message is:

Running RuboCop...
RuboCop failed!
Inspecting 17 files
.........C.......

Offenses:

lib/opentelemetry/instrumentation/http/patches/client.rb:14:39: C: [Correctable] Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect.
          HTTP_STATUS_SUCCESS_RANGE = (100..399).freeze

I think the solution is to remove .freeze since the constant makes the range immutable.

If you'd like to run rubocop locally to test yourself, you can do so using:

docker compose run app
<container_sha>: /app$ cd instrumentation/http_client
<container_sha>: /app/instrumentation/http_client$ bundle install
<container_sha>: /app/instrumentation/http_client$ bundle exec rubocop

[Victorsesan]

Thanks very much for reviewing @kaylareopelle . I have taken a look at the failures again from my end and have them sorted. Some end-of-line characters & carriage return test failures were also found which i converted with dos2unix and ran bundle exec rubocop --autocorrect on them to ensure all the offense in some files were fixed, so we can have some few modification/corrections shown in some files after the test was carried out. Hopefully it was for the best , but if these additional fix aren't needed i will have them regress asap.
Edit: Also very much open to help with any suggestions if needed. Thanks for your time