Freeze all range objects

[hannahramadan]

Freezing objects in Ruby provides several benefits including performance improvements and semantic clarity.

Since ranges are not frozen in Ruby by default, it would be nice to freeze ranges that appear through the codebase.

[Victorsesan]

@kaylareopelle Is this still up for grabs?, can you assign me to give it a try thanks

[kaylareopelle]

Hi @Victorsesan, it is! Just assigned you to the issue. Let us know if there's anything we can do to help 🙂

[Victorsesan]

@kaylareopelle Cool thanks! Just wondering are the required ranges to be frozen that of Inclusive & Exclusive (100..399)? And can we create a different file in the lib directory to write a code to freeze the ranges in , also should we have the entire range as a single object frozen or have an implementation which creates individual frozen ranges?

[kaylareopelle]

Hi @Victorsesan! Inclusive and exclusive ranges can both be frozen as part of this PR.

There's many approaches you could take to solve the problem!

For this scenario, my recommendation would be to create a constant equal to the range and replace the reference to the range within the code with the new constant.

For example, in the HTTPClient instrumentation, the annotate_with_span_response method uses a range, 100..399

opentelemetry-ruby-contrib/instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb

Lines 39 to 46 in 7141afa

	def annotate_span_with_response!(span, response)
	return unless response&.status_code
	status_code = response.status_code.to_i
	span.set_attribute('http.status_code', status_code)
	span.status = OpenTelemetry::Trace::Status.error unless (100..399).cover?(status_code.to_i)
	end

You could add a constant to the top of the OpenTelemetry::Instrumentation::HttpClient::Patches::Client class in the instrumentation/http_client/lib/opentelemetry/instrumentation/http_client/patches/client.rb file, maybe named something like NON_ERROR_STATUS_CODES that's equal to the range, and replace the reference to the range within the method to use the constant instead.

+        NON_ERROR_STATUS_CODES = 100..399

          def annotate_span_with_response!(span, response)
            return unless response&.status_code

            status_code = response.status_code.to_i

            span.set_attribute('http.status_code', status_code)
-           span.status = OpenTelemetry::Trace::Status.error unless (100.399).cover?(status_code.to_i)
+           span.status = OpenTelemetry::Trace::Status.error unless NON_ERROR_STATUS_CODES.cover?(status_code.to_i)
          end

[kaylareopelle]

@Victorsesan - I found a good example of this in the Rack instrumentation:
https://github.com/open-telemetry/opentelemetry-ruby-contrib/blob/main/instrumentation/rack/lib/opentelemetry/instrumentation/rack/middlewares/event_handler.rb#L46

[Victorsesan]

@kaylareopelle Just tried this out! I'm not sure how it was directed to a different section for review 😅hopefully i didn't do something wrong . Anyways please check it out let me know what you think, i used a constant instead of a .freeze method as requested to make the code more readable. I'm very much open for any corrections or improvement if needed

[kaylareopelle]

Hi @Victorsesan, thank you for opening the PR! The changes look like they're on the right track!

Our contributing guide has a few recommendations about making modifications that I would like you to follow before we move forward.

1. Because we squash commits when we merge PRs in this repository, creating PRs off of your fork's main branch can cause problems. Would you mind closing your PR, creating a new branch, moving your changes to that branch, and opening a new PR from that branch?

2. Also, we like to have our PR titles and commit messages follow the conventional commits format. Could you write the commit message on your new branch using a conventional commit style?

If you need more help with any of these steps, please let me know! 😄

[kaylareopelle]

Hi @Victorsesan! Thanks for your work on this! I'm going to respond to your latest comment on your old PR here. Until you open a new PR, let's communicate on this issue!

I'm sorry to hear you're having trouble with Bundler locally. Setting up Ruby on a local environment can be challenging.

To help get around this, we provide Docker containers through docker compose that can be used for development. Could you try following the instructions here and see if that helps? Keep in mind that on the latest version of the docker CLI, the command for compose is now docker compose, not docker-compose, so the instructions may be slightly outdated. Can you give that a try and let me know how it goes?

As far as the commit message goes, I think that looks pretty good!

• I would personally lean toward using refactor instead of fix for this, but that's debatable 😄 Here's a nice breakdown of each commit type.
• Commit messages are usually written in the imperative mood, so in this case, you would use Create instead of Created. I like this guide for writing commit messages.

[Victorsesan]

@kaylareopelle This could work, thanks! I'm currently about to install the dependencies , but just wondering, since they didn't mention what type of service to run in the docker instructions, my guess was in the docker-compose.yml we have something like service: app: below so i tried

docker compose run app bundle install

and its been close to an hour now its still running, its been so long i feel like I'm running a bundle to build a docker app instead. Does it usually take this long, or more importantly did i run the right command? I'm just this close to get everything setup🚀

[kaylareopelle]

Hi @Victorsesan, thanks for giving this a try! It shouldn't take an hour, though I suppose it could for the first time if the internet connection is slow.

As far as the command you shared, that should work, but it'll just run bundle install on the topmost Gemfile.

To test things for a specific instrumentation, I would recommend running docker compose run app instead. When Docker finishes loading, it'll put you into a bash session with all of the available services running, so you should be able to cd into a specific instrumentation and bundle/test things there.

[Victorsesan]

@kaylareopelle Sadly went the same direction again just like the other we had before after many tries 🥲, just can't seem to pass the permission issue.

I even tried changing the permission with the commands in the last line but it still showed the same error like that above.

This was after i had successfully had docker compose run app completed and later on started the docker bundle exec rake test

[kaylareopelle]

I appreciate you continuing to give this a try, @Victorsesan! Thank you for the screenshots.

It looks like the bundle install --gemfile Gemfile command is being run in your main shell, rather than in a bash session inside the docker container. This may be why the path problems continue to happen.

You could try one of the following:

1. If you use docker compose run app bundle exec rake test, all future commands should also begin with docker compose run app, to make sure they are executed inside the docker container rather than directly on your machine.

2. Recommended If you run docker compose run app, you should see something like this after all the services spin up:

Running your commands within the /app$ prompt will run them within the docker container, which should give you access to bundler without permissions issues.

Also, to run the tests, you'll need to be in the directory for a specific instrumentation. Right now, it looks like the commands are being run from the topmost folder in the repository which does not have any tests.

If you take the second approach, within the docker container bash session, you could run the tests using:

<container_sha>: /app$ cd instrumentation/http_client
<container_sha>: /app/instrumentation/http_client$ bundle install
<container_sha>: /app/instrumentation/http_client$ bundle exec appraisal install # this is to install dependencies specific to the test suite
<container_sha>: /app/instrumentation/http_client$ bundle exec appraisal rake test

[Victorsesan]

Finally i succeeded🎉!! @kaylareopelle , after days of trying with continuous fails and errors finally seeing this test pass feels like i just won a lottery fr

Alright then, hopefully that was a good test though 😅, i can't help but feel like something is off still with the Warnings and all, looking forward to know your take on this thanks . To the next step i will be opening my PR now with my new feature branch and add the commits as directed.

Also just wondering is it okay if we can update the contrib.md with some of the test process we've gone through which aren't documented yet? most especially a way to pass the permission errors which i have notice are found in all of the test , not only in docker but in robocop too. I will be happy to help add some few pointers and you review, but if not necessary fine still.

[kaylareopelle]

Yay!! Congratulations! 🎉 🎉

Those warnings are related to the code within the HTTPClient gem itself, so we don't have control over them. The Ruby language is trying to be helpful and letting us know about the state of some variables that might have unexpected behavior.

Updating the documentation would be great! Feel free to open a PR or an issue to update the content to help others who encounter these problems in the future!

[github-actions]

👋 This issue has been marked as stale because it has been open with no activity. You can: comment on the issue or remove the stale label to hold stale off for a while, add the keep label to hold stale off permanently, or do nothing. If you do nothing this issue will be closed eventually by the stale bot.