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 endpoint as a tag on HTTP events #142

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nickdichev
Copy link

Change description

What problem does this solve?

Adds the Phoenix endpoint as a tag value for HTTP metrics. This change allows the panels to take into account the value of the "Endpoint" dropdown on the Phoenix dashboard.

Issue number: (if applicable)
Slack question: https://elixir-lang.slack.com/archives/C01NZ0FBFSR/p1652968317581179

Additional details and screenshots

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.

I have a few questions so I have opened this PR as a draft.

  1. Is there a better way to fetch the Endpoint for a standard conn than pulling it out of the :private key? Obviously, this is not a public API I am reaching into.
  2. There seems to be a discrepancy between the name of the endpoints that the tests declare, TestApp.Endpoint, TestApp.Endpoint2 and the generated events which are being used by the tests. Both support/events/phoenix.exs and support/events/phoenix_multi_router.exs have data for an endpoint from an app named MyAppWeb. This means the tests are wrong. I expect to see TestApp.Endpoint2 as the endpoint tag for requests sent to /internal/*. What is the best way to re-generate the sample metrics?
  3. To test the changes E2E I updated the dashboard JSON manually on two panel. This feels very error prone. Is there a better way to add these changes across all the corresponding panels?
  4. Does this change need documentation? If so: where should it go?

@akoutmos
Copy link
Owner

Thanks for opening up the PR Nick. Really appreciate the contribution :). Below are my answers for all the questions that you had:

  1. As far as I know, there is no other way to pull the Endpoint from the Conn struct. Luckily that code hasn't changed in 5 years in Phoenix source...but it would be great to have a function to officially do it. Perhaps we open a PR to Phoenix to add this functionality in.
  2. Regenerating the stream of test events is a bit of a manual process. You need to run the example application with only the plugin that you are recreating test events for, and then you need to extract the events from the Recorder module: https://github.com/akoutmos/prom_ex/blob/master/example_applications/web_app/lib/web_app/recorder.ex
  3. At the moment, the best way to validate that your dashboard changes haven't broken the dashboard is to make your changes to the JSON EEx template, and then run the example application in the repo. It looks like Grafana has recently been working on a Dashboard linter. This would probably be great to introduce into PromEx, so I'll add it to the todo list.
  4. I don't think any documentation is specifically needed for this. The end result will be that the dropdown in Grafana now filters all panels in the dashboard.

Hope that helps!

@nickdichev
Copy link
Author

nickdichev commented Jun 4, 2022

@akoutmos Thanks for the feedback!

I added the $endpoint label (not sure if thats the proper PromQL name for this thing?) to the remaining Phoenix dashboard panels.

As for re-generating the test events. I think I have a sense of what needs to be done; but have two more questions.

  1. Is there any documentation on using the Recorder? I'm not sure what I need to pass through as the value of the :metrics option and any optional GenServer opts. Though looking at the implementation -- I don't think there's any GenServer specific opts I would need.
  2. Is there a script or some method to send a bunch of the expected requests? Or is the heuristic "send a bunch of requests by clicking around the ui and manually send some requests to the additional_routes"?

Cheers 🤙

@coveralls
Copy link

coveralls commented Jun 10, 2022

Coverage Status

Coverage decreased (-0.06%) to 79.561% when pulling 29caa75 on nickdichev:feature/phoenix-endpoint-tag into 9dbab16 on akoutmos:master.

@nickdichev
Copy link
Author

Hey @akoutmos was just hoping to follow up on my questions above :)

@akoutmos
Copy link
Owner

akoutmos commented Nov 7, 2022

Thanks for pinging me on Slack, I must have missed your messages in my inbox -_-.

Below are the answers to your questions:

  1. That should be taken care of for you via WebApp.RecorderSupervisor. All I would suggest doing is commenting out all other plugins aside from Phoenix and then run WebApp.Recorder. get_events () after you have interacted with the application for a little bit.
  2. Unfortunately there is nothing fancy here. The existing test events were generated by just clicking around and navigating through the example app.

Hope that helps!

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.

3 participants