Adds support for tracking http.rb HTTP requests

[stefanvermaas]

While investigating slow HTTP requests in a project, I noticed I didn't see any of the HTTP requests in AppSignal. After some digging I found #770 and it seems that the http.rb gem that we're using wasn't supported yet.

This PR adds support for http.rb and fixes #770.

I've chosen to name all links to the http.rb gem http_rb (or the capitalized version of it), but the filenames are all without the extra rb suffix. I'm not sure what's the best choice here to make. I'm hoping for feedback on that.

In the meantime, I'll start using the updated version of the gem in our own project to see if everything is reported accurately.

[stefanvermaas]

I’ll check Semaphore. I bet it has to do with the changes to the build matrix.

[shairyar]

Hey @stefanvermaas, thanks for submitting the PR. We will take a look 👀

[stefanvermaas]

It seems the .semaphore/semaphore.yml needs to be altered by running rake build_matrix:semaphore:generate.

I've run this locally and it alters the .semaphore/semaphore.yml correctly, but I'm unsure whether or not I've taken the right approach for the test cases. Would you be so kind to tell me whether or not the test cases are implemented in the way that AppSignal expects them?

---

NB: I also noticed the commit messages were not written in the expected format. I'll adjust them after making sure the test cases are set up correctly etc.

[tombruijn]

It seems the .semaphore/semaphore.yml needs to be altered by running rake build_matrix:semaphore:generate.

Yes, that's needed to run. I don't see the updated .semaphore/semaphore.yml file committed in this PR. If you commit it and push it, the CI will run.

[tombruijn]

@stefanvermaas let me know if you need any further help with this PR :)

[stefanvermaas]

Alright, no worries! I'll make the updates. They sound pretty straightforward. I'll let you know if help is needed.

[stefanvermaas]

@tombruijn pushed additional changes and updated the test cases. Let me know if this is how you'd like to see it and I'll start addressing the comments from "Lintje".

[tombruijn]

Looks good! I just have a couple thoughts, for I also left some suggestions on how implement that.

Let me know if you need help with any of that.

And about Lintje, we can also squash merge it if you want with a fresh message so you don't need to worry about that now. But if you do, please rebase with updated commit messages.

[stefanvermaas]

@tombruijn I do think I need some additional guidance.

When I remove the ensure block which marks the transaction as completed, the specs start to fail; the transaction isn't completed.

When I do manually mark it as completed (see below), I need to continue using around { |example| keep_transactions { example.run } }, while your suggestion is to remove it.

If I don't include the keep_transactions, the tests will start to fail too on the Appsignal::Transaction#to_h call as the @ext instance variable seems to be nil. I'm not sure why it's being nil as I don't think it should be.

Should I test for completeness or only for the events?

I also noticed that the net_http specs don't test for completeness, and the que specs do. However, the que integration explicitly completes the transaction too and that's why it's marked as completed. For the net_http specs, it only asserts on start and finish events.

# Manually mark the transaction as completed.
begin
  Appsignal.instrument("request.http_rb", "#{verb.upcase} #{uri}") do
    super
  end
rescue Exception => error # rubocop:disable Lint/RescueException
  Appsignal.set_error(error)
  raise error
ensure
  Appsignal::Transaction.complete_current!
end

[tombruijn]

@stefanvermaas

When I remove the ensure block which marks the transaction as completed, the specs start to fail; the transaction isn't completed.

That's correct. The HTTP.rb instrumentation shouldn't complete the transaction. It should be recorded as part of another transaction.

A HTTP.rb request is most likely made as part of a Rake task, background job or in a Rails request.

In a que job or rails request, the instrumentation for that gem creates and completes the transaction. It's responsible for the transaction. A que job or rails request, is a chunk of work which we record in a transaction. In this transaction we record events like view rendering, database queries, etc. Those are events in the transaction.

A HTTP.rb request is recorded as an event on the transaction. It's not something that we track as its own transaction. Otherwise we'd create new incidents on AppSignal.com for every request made. (It would also break the transactions from the higher level instrumentation like que and rails.)

Also consider if a new transaction for a HTTP.rb request has any events in them, other than just the request.http_rb event. It will do nothing else than that one event. So it's better to track it as part of another transaction.

Example transaction structure:

• Transaction: Rails request, web namespace, HelloController#show
  • Events:
    • process_action.action_controller
    • query.active_record
    • render_layout.action_view
    • request.http_rb <-- This is where we want to see the HTTP.rb event, not as a new transaction

If I don't include the keep_transactions, the tests will start to fail too on the Appsignal::Transaction#to_h call as the @ext instance variable seems to be nil. I'm not sure why it's being nil as I don't think it should be.

Now that I look at it closer, it does more than just keep them in memory. But it also tells the test suite to sample the transaction, which allows to_h to return data. So it is necessary to use keep_transactions there. Sorry about that confusion.

I also noticed that the net_http specs don't test for completeness, and the que specs do. However, the que integration explicitly completes the transaction too and that's why it's marked as completed. For the net_http specs, it only asserts on start and finish events.

The difference between the que and net http instrumentations are that a que job is an entire transaction, and the net http request is an event of a larger transaction (started by a que job or rails request). So it's correct that que completes the transaction and the net http (and http.rb) instrumentations do not.

[stefanvermaas]

That's correct. The HTTP.rb instrumentation shouldn't complete the transaction. It should be recorded as part of another transaction.

A HTTP.rb request is most likely made as part of a Rake task, background job or in a Rails request.

In a que job or rails request, the instrumentation for that gem creates and completes the transaction. It's responsible for the transaction. A que job or rails request, is a chunk of work which we record in a transaction. In this transaction we record events like view rendering, database queries, etc. Those are events in the transaction.

A HTTP.rb request is recorded as an event on the transaction. It's not something that we track as its own transaction. Otherwise we'd create new incidents on AppSignal.com for every request made. (It would also break the transactions from the higher level instrumentation like que and rails.)

That entirely makes sense, thanks. That was for me the missing piece. I wasn't entirely sure what "complete" meant in this situation, but I do understand now.

Now that I look at it closer, it does more than just keep them in memory. But it also tells the test suite to sample the transaction, which allows to_h to return data. So it is necessary to use keep_transactions there. Sorry about that confusion.

No worries, that makes sense too. I'll update the specs.

Thanks for the feedback and the explainer about the internals.

[stefanvermaas]

@tombruijn all requested adjustments are made. Feel free to review it again.

[tombruijn]

I've created PR appsignal/diagnose_tests#75 for the added config option that now fails the diagnose tests. For a green build we need to wait for #890 to be merged because that also adds a config option and that has already been merged in the diagnose tests repo's main branch.

[tombruijn]

Looks really good! I have a couple minor comments. I'll try to find some time to test this in an app and see if it all comes in as expected.

[tombruijn]

Looks like it works. I tested it in a small test app.

I'll fix the colors for the event.

[tombruijn]

Colors and grouping for the http_rb event group have been merged. It will show up in the "Slow requests" feature on the next AppSignal.com deploy.

[stefanvermaas]

That's really awesome! Thanks! I noticed the integration tests start to fail, but I'm not able to replicate it locally. Any ideas?

[tombruijn]

I fixed the tests in another repo, but we need to update it before merge. Updating it cause other errors. I mentioned this in a previous comment. #889 (comment)

I'll review your latest changes and check internally what's the best next step in relation to the other PR.

[tombruijn]

@stefanvermaas we have merged PR #890. That includes some updates to our diagnose_tests repo. If you're up for it can you rebase this PR on the latest main branch, update the submodule, and force push it so it's up-to-date? If not, I can do it for you.

I think it's this collection of commands you need to run:

git submodule init
git submodule update --init --recursive
cd spec/integration/diagnose
git fetch origin
git checkout instrument_http_rb

Commit your changes and push the updated Git submodule in the appsignal-ruby repo.

[stefanvermaas]

Hmm, it keeps failing on me while this seems right to me.

Would you mind making the changes needed?

[tombruijn]

@stefanvermaas I think that's a mistake on our end. The config option that the diagnose_tests specs expect wasn't configured with a default value.

I created a fix in PR #893. Let's see what the team says. If approved and merged you'll need to rebase on that merged version.

[tombruijn]

@stefanvermaas I've merged PR #893. You can rebase on the latest main branch. Then.. now really.. it should work 😅

[stefanvermaas]

Well, well, well. Look at that 🙌 It seems we're good to go!

[stefanvermaas]

Thanks for the patience and support @tombruijn!

[tombruijn]

@stefanvermaas Thanks again for the PR 🥳 I've released it in Ruby gem version 3.2.0 just now. I'll update the docs website.

[tombruijn]

@stefanvermaas I forgot to mention this earlier, but we'd like to send you something as a thank you for your work! Please send a message on support@appsignal.com with your address and a link to this issue 😄