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

use PhoenixProfiler on the Endpoint #58

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

use PhoenixProfiler on the Endpoint #58

wants to merge 40 commits into from

Conversation

mcrumm
Copy link
Owner

@mcrumm mcrumm commented Mar 3, 2022

No description provided.

%Profile{data: %{metrics: metrics}} = profile

socket =
case profile.data[:exception] do
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like there's a race condition here, the toolbar is mounted before telemetry has a chance to collect the event [:phoenix_profiler, :endpoint, :exception] so profile.data[:exception] is always null at this point.

I'm assuming that http://localhost:4000/errors/assign-not-available should populate @exits considering

telemetry_execute(:exception, %{duration: duration(start_time)}, %{
and
{:keep, %{exception: Map.take(meta, [:kind, :reason, :stacktrace])}}

@mcrumm
Copy link
Owner Author

mcrumm commented Mar 4, 2022

@leandrocp You are correct– the toolbar cannot render the error for its own request because the Phoenix.Endpoint.RenderErrors module takes over sending the response. To account for this, the private late_collect/3 function on the profiler endpoint is invoked after the response has been sent, which allows us to collect additional information about the request to be viewed later.

I think you are also correct about the race condition tho– we are racing the late write to ETS and that's why we are getting back profile data without the exception.

But now I think I have an idea of how to get the tests passing, stand by! :)

The endpoint integration tests are racing the late collection
behaviour in the profiler endpoint, which sometimes results in
tests failing due to receiving outdated profile information.

We are attempting to correct this by fetching the profile data
by its token thru another endpoint request. Hopefully this will
slow things down enough to give the late write a much better chance
to finish before the data is requested.

In practice this isn't an actual problem, as in most cases (like the
Dashboard) we poll for updated information.
Copy link
Collaborator

@leandrocp leandrocp left a comment

Choose a reason for hiding this comment

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

Hey @mcrumm just tested this PR on a real project and it looks good to me 👍🏻

@mcrumm
Copy link
Owner Author

mcrumm commented Mar 11, 2022

@leandrocp Thanks for testing it out! Yes, things seem to work fine but the Heisenbugs are forcing me to question some earlier decisions. I just had a nice chat with @elbow-jason who gave me some ideas that I hope to experiment with soon! :)

* main:
  Update version in README
  Release v0.2.0
  Update supported LiveView versions (#65)
  Update dev.exs
  Add troubleshooting for on_mount, closes #61
  Simulate hooks on demo app
  Handle multiple body tags
@mcrumm mcrumm mentioned this pull request Feb 20, 2023
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.

2 participants