Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add response status code to transaction metadata #183

Closed
7 tasks done
matsimitsu opened this issue Oct 6, 2016 · 12 comments
Closed
7 tasks done

Add response status code to transaction metadata #183

matsimitsu opened this issue Oct 6, 2016 · 12 comments
Assignees
@tombruijn
Labels
enhancement

Comments

@matsimitsu
Copy link
Member

matsimitsu commented Oct 6, 2016 •
edited by tombruijn
Loading

Response status codes (200,202 etc.) could help debug performance issues. e.g. short-returning a 404-not found could have a different impact on your app then returning a 200 with a body. While it might be clear from the event timeline, I think it's nice to have as additional metadata.

Blocked by #329

Instrumentations

Response status reporting will be supported in #1090 through the Rack EventHandler. This will only work for Rails first, as it's the first instrumentation to use that. As we port other instrumentations over to use Rack EventHandler, more libraries will be supported.
@tombruijn
Copy link
Member

We can get the status code from the @app.call(env) response like in the code below when we would add it to the rails_instrumentation.rb

@app.call(env).tap do |status, _, __|
  transaction.set_metadata("status", status)
end

This would work for any Rack application since they are required to return the response triplet: status code, headers, and body (in that order). So we can add it for Rack, Sinatra and Grape too.

Is this what you had in mind, @matsimitsu ?

@matsimitsu
Copy link
Member Author

Sounds good! 👍

@tombruijn tombruijn self-assigned this Oct 25, 2016
@tombruijn tombruijn added this to the 2.3 milestone Mar 8, 2017
@tombruijn tombruijn modified the milestones: Ruby gem 2.4, Ruby gem 2.3 Jun 13, 2017
@tombruijn tombruijn removed their assignment Oct 26, 2017
@tombruijn tombruijn changed the title Add response status code to transaction metadata (Rails) ⌨️ Add response status code to transaction metadata (Rails) Oct 27, 2017
@tombruijn tombruijn changed the title ⌨️ Add response status code to transaction metadata (Rails) Add response status code to transaction metadata (Rails) May 30, 2018
@tombruijn tombruijn removed this from the The next minor Ruby gem version milestone Jan 7, 2019
@jeffkreeftmeijer jeffkreeftmeijer mentioned this issue Jan 28, 2019
Merged
@phoet
Copy link

phoet commented May 13, 2019

@tombruijn so this is implemented for the elexir client but not in the ruby version?

@tombruijn
Copy link
Member

@phoet correct. For us to capture all response codes we need to rework how our middleware works. Currently we would only get part of the response codes. When there’s middleware before AppSignal’s we would not track them, giving an incomplete picture.

We want to improve that in issue #329

@tombruijn tombruijn mentioned this issue Jan 10, 2020
Closed
36 tasks
@richardonrails
Copy link

This would be very helpful. I was hoping to see throughput and response time graphs by HTTP response code out-of-the box with AppSignal.

tombruijn added a commit that referenced this issue May 31, 2024
@tombruijn
Improve Rails/Rack request instrumentation
0fe2edc 
Rework the Rails instrumentation to use the Rack::Events module to start
and complete the transaction at the very start and end of the request.
A new middleware is added at the very top of the middleware stack to
start and complete the transaction.

This allows us to instrument more of the request's duration, as we're
now at the very start and end of the request handling. We will now
report the time the request has spend handling any other middleware.
This will be visible with a new `process_request.rack` event.

To make sure we always close the transaction, listen for the `on_finish`
event, but also register a last resort "after_reply" callback. This
callback is supported by web servers like Puma and Unicorn. If it's
called when a transaction is active, it will close it, preventing
transactions to span multiple requests. If no transaction is active, it
will do nothing.

Moving forward, I'd like to update our Rack, Sinatra, Padrino, Grape,
etc. instrumentations to use this EventHandler middleware too, which
will be a bit more work, because some require users to manually add
middleware. When they all use the same EventHandler, we have one place
to start and complete transactions, and issues like
#289 cannot happen
again in the future.

Previously, our only Rails instrumentation was positioned after the
DebugExceptions middleware, just in time to catch any errors before the
Rails error middleware showed a nice error/debug page. This wouldn't
show the complete picture in request duration.

The original Rails instrumentation middleware will remain to report
errors and Rails specific metadata. This cannot be part of the
EventHandler, without overloading it with responsibilities, and for
Rails error reporting we need to be right after the DebugExceptions
middleware before errors are no longer bubbled up in the stack.

TODO: SPLIT COMMIT:
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack returned another status.

This is part of issue
#183, but only
implements it now for Rails apps and apps using the
`Appsignal::Rack::EventHandler` manually. We'll need to update other
instrumentations for Rack, Sinatra, Padrino, Grape, etc. to use this new
EventHandler, so they can also benefit from this new response status tag
and metric.

TODO:

- How to report the request_id? Can't overwrite the tag
- Remove the `request_id2` tag
@tombruijn tombruijn self-assigned this May 31, 2024
tombruijn added a commit that referenced this issue Jun 3, 2024
@tombruijn
Report request response status for Rails apps
211ab2f 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
tombruijn added a commit that referenced this issue Jun 3, 2024
@tombruijn
Report request response status for Rails apps
514f4f2 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
@tombruijn tombruijn mentioned this issue Jun 3, 2024
Merged
tombruijn added a commit that referenced this issue Jun 3, 2024
@tombruijn
Report request response status for Rails apps
d0db72b 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
@tombruijn
Copy link
Member

tombruijn commented Jun 7, 2024 •
edited
Loading

Requested in this Intercom conversation (2024-05-28).

Working on this in PR #1090

tombruijn added a commit that referenced this issue Jun 17, 2024
@tombruijn
Report request response status for Rails apps
2ded927 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
tombruijn added a commit that referenced this issue Jun 17, 2024
@tombruijn
Report request response status for Rails apps
700db53 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
tombruijn added a commit that referenced this issue Jun 17, 2024
@tombruijn
Report request response status for Rails apps (#1090)
5459a02 
We can now report the response status as a `response_status` tag and
metric. Previously, due to the position of the Rails middleware, this
was unreliable and wouldn't always report the real status code if other
middleware in the stack that are run after ours returned another status.

This is part of issue #183, but only implements it now for Rails apps
and apps using the `Appsignal::Rack::EventHandler` manually. We'll need
to update other instrumentations for Rack, Sinatra, Padrino, Grape, etc.
to use this new EventHandler, so they can also benefit from this new
response status tag and metric.
@tombruijn
Copy link
Member

We report the response_status metric for the response status since Ruby gem 3.8.1 for Rails apps. Other frameworks coming soon. You can now create a graph on a dashboard to show this metric.

@phoet
Copy link

phoet commented Jun 17, 2024

💋

@phoet
Copy link

phoet commented Jun 17, 2024

Hm, I'm not able to create a dashboard with metrics from response_status. I do see the status-code in the performance samples as a tag though 🤔

@tombruijn
Copy link
Member

You should be able to make a graph like this:
image

If the tag is set on samples, the metric is sent as well. These things are set right after one another.

@phoet
Copy link

phoet commented Jun 17, 2024

ah, i was missing the namespace part 👍

@tombruijn tombruijn changed the title Add response status code to transaction metadata (Rails) Add response status code to transaction metadata Jul 4, 2024
@tombruijn
Copy link
Member

This has been released in several recent Ruby gems, with the last libraries supported in Ruby gem 3.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tombruijn tombruijn

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@matsimitsu @phoet @tombruijn @richardonrails