[BUG] Phoenix plugin: path is lost when router forwards to another router

[leolaudouard]

When using forward function from a router to forward to another router, the resulting metric will have the first router route as path.

Maybe I missed some documentation on how to configure for this use case, sorry if it is the case.

To Reproduce

I slightly modified the example application to reproduce this issue:

leolaudouard@b5b2ac9

With these routers, the resulting path is always /users, whereas before we had /users/:id

Expected behavior

Same path values as when the router directly forwards to controllers.

I suppose the issue is around here.

[aerosol]

it seems that also the controller is always the router we forward to, and action is "Unknown"

[aerosol]

plausible_prom_ex_phoenix_http_request_duration_milliseconds_bucket{action="Unknown",controller="PlausibleWeb.Plugins.API.Router",host="localhost",method="GET",path="/api/plugins",status="401",le="+Inf"} 1

[aerosol]

@akoutmos I might have a patch for this, any hints on how to test it? I hope we're not expected to craft https://github.com/aerosol/prom_ex/blob/master/test/support/events/phoenix.exs by hand? 🤢

[akoutmos]

Hey there! Sorry I missed this issue. Those fixtures are automatically generated via https://github.com/akoutmos/prom_ex/blob/master/example_applications/web_app/lib/web_app/recorder_supervisor.ex

The way I have been creating them is by adding that to the example app, enabling only the PromEx plugins I want to test, and then interacting with the app in a way that triggers the telemetry events I want to test.

You can then fetch all of the recorded events via https://github.com/akoutmos/prom_ex/blob/master/example_applications/web_app/lib/web_app/recorder.ex

It's not the easiest thing in the world to work with....but does the job. If you make the fix, I can add to the test suite if that makes it easier :)

[aerosol]

Ugh, it's a bit more involved with the current setup. I've spent a lot of time on this and I don't think I can pull it off reasonably well. Some notes in case someone wants to tackle it:

It's a bit of a vendor lock-in situation 😅 - since we can't stop using PromEx easily (due to existing metrics/graphs in a 3rd party storage that are expensive to replicate/replace), the best solution in terms of effort/reward will be to stop using forward and merge the routers. At least, as long as this library stays up to date with all the optional dependencies it aims to target.

• I managed to use the Recorder dumps but the static fixtures are difficult to deal with, big file in, expect big file out, hard to pin-point bugs from test failures as you go, especially with errors such as 142 != 168
• as far as I understand, having only Plug.Conn available, forwarded-to routers can be recognized only by looking up opaque bits: conn.private or function_exported?(plug, __routes__, 0) to determine a match on a router-plug vs controller-plug
• if we go for conn.private, there's conn.private.phoenix_router available, pointing at the specific plug, but the path segments are split with each forward (as per scope). Relevant info can be found in conn.private[first_router] => {[], %{second_router => prefix}} and conn.private[second_router] => {prefix, %{}}.
• note that forwards can be embedded indefinitely, so we'll have to recursively deal with the shrinking path prefix for the final Phoenix.Router.route_info/4 lookup
• using private assigns invalidates all the test fixtures. WebAppWeb.* module names coming from the OG recordings cannot be relied upon anymore (it's TestApp.* now)
• we're using newer Phoenix on prod and /surprised pikachu face/ the private router annotation values come in different shapes than what I'm seeing in this setup, e.g.

private: %{
  :open_api_spex => %{spec_module: PlausibleWeb.Plugins.API.Spec},
  PlausibleWeb.Router => [],
  PlausibleWeb.Plugins.API.Router => ["api", "plugins"], # <-- derp
  :before_send => [#Function<0.54455629/1 in Plug.Telemetry.call/2>,
   #Function<1.492364/1 in Phoenix.LiveReloader.before_send_inject_reloader/3>],
  :phoenix_endpoint => PlausibleWeb.Endpoint,
  :phoenix_router => PlausibleWeb.Plugins.API.Router,
  :plug_session_fetch => #Function<1.76384852/1 in Plug.Session.fetch_session/1>,
  :phoenix_format => "json"
}

Hope this helps someone who is clever enough.