Add body metadata to instrumentation

[braktar]

Hi !

While using the rails_performance project to analyze the queries triggered in my API and passed through ActiveSupport::Notifications, I noticed that several queries were not being logged.

After digging into the issue, I found that only the "format_response.grape" event was being captured. However, this event is not always emitted by Grape — in particular for responses without content. In those cases, the payload contains no information about the body, which makes the notification sequence unpredictable.

To make the behavior of Notifications more predictable, I added some extra information related to the bodies.

Does this approach make sense?

[dblock]

The goal makes a lot of sense: we want the information about the response body. Thanks for picking this up!

The implementation needs some work I think.

We instrument request, therefore I think we should be instrumenting response, which then contains things like status code, content-length or content-encoding in headers. We don't want to rename these things (get rid of is_stream or has_entity_body), and stick as close as possible to the HTTP protocol, so response and headers (should include things like content-length). Those should provide the same information, but would be future proof and aren't grape-internal-specific.

We'd want

ActiveSupport::Notifications.instrument(
  'endpoint_run_validators.grape', 
  endpoint: self, 
  validators: validators,
  request: request)

to become

ActiveSupport::Notifications.instrument(
  'endpoint_run_validators.grape',
  endpoint: self,
  validators: validators,
  request: request,
  response: response)

Give it an iteration along those lines?

[braktar]

Great ! Thanks for the feedback and the guidance. I will definitely give it another iteration

[ericproulx]

Hi, @braktar, almost all notification has an endpoint in their payload so that you build can build metadata from it when processing events. The only exception (which I will fix) is for format_response that does not have an endpoint but just a env. Fortunately, you can access it with Grape::Env::API_ENDPOINT. Here's a script that prints some data based on your spec

# frozen_string_literal: true

require 'bundler/inline'

gemfile(true) do
  source 'https://rubygems.org'
  gem 'grape'
  gem 'rack'
end

require 'grape'
require 'active_support/notifications'

class MyApi < Grape::API
  format :json

  get '/simple' do
    { message: 'Hello World' }
  end

  get '/stream' do
    stream StringIO.new('streamed content')
  end

  get '/file' do
    sendfile __FILE__
  end

  get '/empty' do
    status 204
    ''
  end

  get '/with_params' do
    params do
      requires :id, type: Integer
    end
    { id: params[:id] }
  end

  get '/array_body' do
    [1, 2, 3, 4, 5]
  end

  get '/string_body' do
    'Hello World'
  end

  get '/hash_body' do
    { users: [{ id: 1, name: 'John' }, { id: 2, name: 'Jane' }] }
  end
end

events = []
ActiveSupport::Notifications.subscribe(/grape/) do |event|
  events << event
end

data = %w[simple stream file empty array_body string_body hash_body].flat_map do |path|
  env = Rack::MockRequest.env_for("/#{path}")
  MyApi.call(env)
  events.map do |e|
    endpoint = e.payload.key?(:endpoint) ? e.payload[:endpoint] : e.payload[:env][Grape::Env::API_ENDPOINT]
    { path: path, status: endpoint.status, body: endpoint.body, api_format: endpoint.env[Grape::Env::API_FORMAT] }
  end
end
puts data

With endpoint and env so you have access to all the data

[braktar]

Thanks @ericproulx! It means that for my need, I don’t need any modification on the grape side to meet my requirement on rails_performance 👌

[dblock]

@ericproulx Do you think we don't need this as a feature?

[ericproulx]

@ericproulx Do you think we don't need this as a feature?

I think we should update the README to provide concrete example about how to fetch data within the payload so that users can customize. I really think this is what is missing.

[dblock]

@braktar care to help?

[braktar]

Sure ! #2609