Sb8244 request queue #142
Sb8244 request queue #142
Conversation
The headers are always lowercase, so this will resolve any case sensitivity issues
| defp derive_queue_duration(%{start_time: start_time, queue_start_us: queue_start_us} = tx) | ||
| when not is_nil(queue_start_us) do | ||
| start_time_us = System.convert_time_unit(start_time, :native, :microsecond) | ||
| queue_duration_us = max(0, start_time_us - queue_start_us) |
There was a problem hiding this comment.
This is one protection from skew. This could stay, or the other could stay, or both. I think this one is much more performant since it's not going to clock, so it might be a better candidate to stay.
| max_call_time: total_time_s | ||
| } | ||
| ] | ||
| |> queue_duration_metric(queue_duration_s) |
There was a problem hiding this comment.
we should report the queue metric separately from this, in this file we should just be adding a function like this:
def transform(:queue_time, duration_s: duration_s) do
[
%Metric{}
]
end
| @@ -494,7 +510,8 @@ defmodule NewRelic.Transaction.Complete do | |||
| def report_transaction_metric(tx) do | |||
| NewRelic.report_metric({:transaction, tx.name}, | |||
| duration_s: tx.duration_s, | |||
| total_time_s: tx.total_time_s | |||
| total_time_s: tx.total_time_s, | |||
| queue_duration_s: Map.get(tx, :queue_duration_s, nil) | |||
There was a problem hiding this comment.
let's have 2 calls to report_metric here instead of smushing them together
lib/new_relic/transaction/plug.ex
Outdated
| |> NewRelic.add_attributes() | ||
| end | ||
|
|
||
| @queue_duration_headers ["x-request-start", "x-queue-start", "x-middleware-start"] |
There was a problem hiding this comment.
I'd rather just stick with x-request-start than pile on a bunch of extra logic for backwards compatibility
There was a problem hiding this comment.
Happy to make that change. I didn't realize they were legacy
lib/new_relic/transaction/plug.ex
Outdated
| {:halt, us} | ||
|
|
||
| {:error, reason} -> | ||
| Logger.debug(reason) |
There was a problem hiding this comment.
There's a NewRelic logger that'd be preferable, though i'm not sure it's something i'd really want to log out
There was a problem hiding this comment.
I'm happy to remove it. I put it here to mirror the Ruby agent log. I could see someone wondering "why isn't my queueing metric reporting?"
lib/new_relic/transaction/event.ex
Outdated
| @@ -23,7 +24,8 @@ defmodule NewRelic.Transaction.Event do | |||
| timestamp: transaction.timestamp, | |||
| name: transaction.name, | |||
| duration: transaction.duration, | |||
| type: transaction.type | |||
| type: transaction.type, | |||
| queueDuration: transaction.queue_duration | |||
There was a problem hiding this comment.
This isn't where we should be reporting the attribute, only a small set of attributes are accepted here, we can remove this from Event.
| tx | ||
| |> Map.merge(%{ | ||
| queue_duration_us: queue_duration_us, | ||
| queue_duration_s: queue_duration_us / 1_000_000 |
There was a problem hiding this comment.
This is actually where we need to report the final queueDuration attribute... I don't think we need the other time unit attributes, and we should actually drop the queue_start_us attribute
There was a problem hiding this comment.
Oh. So these transaction attributes will be added to the final transaction automatically? The key should be queueDuration in that case?
|
Thanks for contributing! I'm going to follow up with a few commits before pulling this into |
Okay, this is for #141. It's the biggest PR I've done here, so it probably has a good bit that could be changed.
Goals:
queueDurationas seconds (fractional to the microsecond) on web transactions"x-request-start", "x-queue-start", "x-middleware-start", as per the Ruby implementationThe Ruby implementation served as inspiration for deriving queue start, but I didn't handle some cases like having a compound header, or multiple headers with different values.
I haven't tried it in practice, yet.