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.

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

add host tag to phoenix plugin to handle paths depended on a subdomain #183

Merged
merged 4 commits into from
Apr 20, 2023

Conversation

bangalcat
Copy link
Contributor

Change description

Hi, I'd like to resolve the #149 issue and also my case with this PR.

So I changed Phoenix.Router.route_info/4 call and added a host tag(metadata) to http_events of Phoenix plugin. Not only I did just tweak the Phoenix.Router.route_info/4 call but added a tag because if I don't, it can't be distinguishable in metrics even if different subdomains have the same paths and the same controller. Still, I'm not sure about that worth it for others too. Maybe the existing controller tag would be enough for most.

In addition, It seems that tweaking grafana templates is necessary to show a proper dashboard, though I don't know much about grafna templates so pleased for any advice.

What problem does this solve?

Issue number: #149

My case: I'm in an enterprise environment, and we have a large phoenix project. Some paths are allowed to mobile clients, and some paths are allowed to other internal servers which through different pipelines. So it ended up there being different subdomains with the same controller and the same paths.

Example usage

After this PR is merged, you can handle different subdomains paths, so when you have a phoenix router:

defmodule MyWebApp.Router do
  use Phoenix.Router
  ...
  scope "/", MyWebApp do
    pipe_through [:api]
    get "/users", UsersController, :index
  end

  scope "/", MyWebApp, host: "internal." do
    pipe_through [:internal_api]
    get "/users", UsersController, :index
  end
end

You can collect separated metrics for those paths.

Additional details and screenshots

I've tested this on my environments.

Checklist

  • I have added unit tests to cover my changes.
  • I have added documentation to cover my changes.
  • My changes have passed unit tests and have been tested E2E in an example project.

@@ -1079,7 +1079,7 @@
}
},
%{
event: [:phoenix, :endpoint, :stop],
event: [:internal, :endpoint, :stop],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind sharing more about this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I totally forgot about this PR. This was probably just for passing the test case, though I’m not sure. I will look into this again ASAP. Thanks for the comment.

By the way, I made my custom phoenix plugin for this purpose and it has been working great for my team.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, I made my custom phoenix plugin for this purpose and it has been working great for my team.

🚀 🥳 write some blog post or something see we learn about it 🚀 🥳

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some investigation, here's what I've figured out.
Firstly, there are only two endpoints in the test file, as both endpoints define custom event_prefix ([:external, :endpoint], [:internal, :endpoint]). So it seems like there should be only events starting with those prefixes in this file.

Next, in the phoenix_multi_router_test.exs, it looks like that it expect 2 [:internal, :endpoint, :stop] event for "/really-cool-route" that belongs to additional_routes. Before this change, there was only one event of it when it comes to the [:internal, :endpoint, :stop] event.

So how the test could pass before? Well, in my guess, there is one more "/really-cool-route" path event, though it belongs to [:external, :endpoint, :stop]. I think it was caught with the wrong metadata before my changes.

I'm not 100% sure 'cause I didn't make that test originally but I think it would make sense..

@coveralls
Copy link

coveralls commented Apr 20, 2023

Coverage Status

Coverage: 78.731% (+2.6%) from 76.125% when pulling 68ffa26 on bangalcat:patch-1 into b52c787 on akoutmos:master.

@akoutmos akoutmos merged commit 6a002fb into akoutmos:master Apr 20, 2023
@akoutmos
Copy link
Owner

Thank you for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants