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

[kaylareopelle]

Hi @Victorsesan! Thank you for your Rubocop updates! Great work so far!

It looks like your latest commit resolved the Rubocop error only on HTTP Client instrumentation. I should've been more specific in my earlier instructions that they would only run the HTTP Client instrumentation.

The changes in your PR span multiple instrumentation libraries. This means you'll need to change directory into those libraries within the Docker container to run Rubocop on each of them. Alternatively, you could try a find-and-replace to remove the .freeze at the end of the new HTTP_STATUS_SUCCESS_RANGE constants.

If you'd like to try to run Rubocop locally on each of those libraries, you can do so by:

docker compose run app
<container_sha>: /app$ cd instrumentation/<gem name, ex. ethon>
<container_sha>: /app/instrumentation/<gem name>$ bundle install
<container_sha>: /app/instrumentation/<gem name>$ bundle exec rubocop --autocorrect
<container_sha>: /app/instrumentation/<gem name>$ cd ../<next gem name, ex. excon>
<container_sha>: /app/instrumentation/<next gem name>$ bundle install
<container_sha>: /app/instrumentation/<next gem name>$ bundle exec rubocop --autocorrect
# repeat for the remaining gems: faraday, http, httpx, net_http, restclient

[Victorsesan]

Did i hit the mark this time, @kaylareopelle . I might have missed something tho🤔 , i had some offenses which i'm not sure i understand what they're.

[kaylareopelle]

Hi @Victorsesan, this looks great! 🎉

Thank you so much for your patience and willingness to work with me through this.

I think the extra offenses you saw are related to an issue with the way Windows terminates lines. This Stack Overflow might help for the future.