Adds Bandit HTTP server support #442
Conversation
lib/new_relic/telemetry/plug.ex
Outdated
| status: status_code(meta), | ||
| memory_kb: info[:memory] / @kb, | ||
| reductions: info[:reductions], | ||
| "bandit.monotonic_time": meas[:monotonic_time] |> to_ms, |
There was a problem hiding this comment.
Monotonic time only makes sense when used to calculate a duration. It shouldn't be reported as an attribute itself...
| defp get_system_time(%{system_time: system_time}), do: system_time | ||
| defp get_system_time(%{monotonic_time: monotonic_time}), do: monotonic_time |
There was a problem hiding this comment.
We might be conflating these two time types, which are fundamentally different measures. We need to look into the source of both values and make sure we aren't doing that.
I can look closer at this later today...
There was a problem hiding this comment.
You are right @binaryseed .
cowboy uses :erlang.system_time(), on the other hand Bandit uses System.monotonic_time().
Any ideas on how to solve this?
There was a problem hiding this comment.
Looking back on things, I think system_time was probably not necessary to add as an attribute in the first place. The agent already ensures that a start_time is tracked.
It's unlikely that system_time is used, but in the spirit of backward compatibility, I'd suggest that we simply don't include it for Bandit.
You could probably just pass meas to add_start_attrs and only include system_time for cowboy
|
@rodrigocaldeira Would you also be able to take a look at why tests are failing? |
Missing |
Also, I've found the integration tests failed on the command I did change it to |
| if NewRelic.Config.enabled?(), | ||
| do: DistributedTrace.start(:http, meta.req.headers) | ||
| do: DistributedTrace.start(:http, get_headers(meta, server)) |
There was a problem hiding this comment.
We could avoid converting headers to a map twice by gathering the headers into a variable before this line and passing them along to this function and maybe_report_queueing
|
Ideally we test this along with the other The simplest way would probably be to add an additional |
|
I rebased, added tests, and fixed a few issues to this in: #445 |
|
Closing in favor of #445 (which includes all of the commits from this PR) |
Description
This PR aims to add Bandit HTTP server support to New Relic's Elixir agent.
Suggested changes
@bandit_start,@bandit_stop, and@bandit_exceptionPlug events inplug.ex, each of them corresponding to Bandit's telemetry spans emitted by Bandit.Motivation
Currently New Relic's Elixir agent supports plug_cowboy adapter only, and by applying the suggested changes to this lib I was able to collect and send web transactions to New Relic normally.
Related issues
Questions
I didn't find any test directly related to
NewRelic.Telemetry.Plugmodule, so I couldn't find a way to check if the suggested changes would break any feature. Also I didn't find any indicator on the debug logs that could tell me if something was wrong during my tests, so any advise on how to create a test to check this is more than welcome!