chore(Logger): add metadata to logs

[Miradorn]

This adds metadata do the default phoenix log entries (something we're doing in our custom implementation currently).

Also it adds the method and request_path to the phoenix_endpoint_stop log, since without it it's harder to see to which request it belongs.

[Miradorn]

Hi there awesome people!

It seems I'm doing something stupid here, but I can't figure out for the live of me why this change would break the tests the way it does. It would seem like nothing gets logged anymore, but I'm not sure why that would be 🤔 Any help would be appreciated!

[agonzalezro]

Hey @Miradorn, it doesn't seem to be your fault. If you check the master branch it also has this broken test: https://github.com/phoenixframework/phoenix/actions/runs/8099067620/job/22134508566

When it gets fixed, if you rebase, you should be good to go 😄

[Miradorn]

Oh, should have checked there 🙈 thanks for pointing it out!

[josevalim]

While this is a good idea, it does have the downside that metadata might be copied between processes alongside the message. The difference is that the message will be certainly used, while the metadata might not. So I am afraid we should not do this by default. :( you can opt in yourself or use telemetry.

[Miradorn]

Hey Jose, thanks for the insight.

Just for me understanding, in which case might this happen? :D

And three followup questions:

1. would a MR just containing the changed "request stop" message (including message and request_path in it) be considered
2. You're saying I should change this MR to make the included metadata configurable? Or smth else?
3. with or use telemetry you mean overwriting the telemetry handlers for the events in our own code, right?

[josevalim]

Just for me understanding, in which case might this happen? :D

In all Elixir versions prior to v1.15 since the metadata was sent to a GenEvent (a separate process). And now it happens whenever someone uses a custom old Elixir backend.

Right now I am thinking we just shouldn't merge any changes, sorry. :( But...

with or use telemetry you mean overwriting the telemetry handlers for the events in our own code, right?

Yes. Maybe we should accept a PR to disable only some events (today you have to disable them all) if you think that would be useful to you.

[Miradorn]

Interesting, no problem. we're currently already using :telemetry.detach with the Phoenix.Logger for the events we want to override, so not really a problem we need solved. I just thought it might be useful to upstream them.
If currently that's not sensible, no problem! ❤️