introduces worker name option, use label on worker metrics instead

[IndraGunawan]

this PR introduces a new optional name config under frankenphp.worker this change is inspired by Caddy server name (https://caddyserver.com/docs/caddyfile/options#name), and this name config will be used in metrics and logs related to worker

as this config is optional, the default value is worker.file

this also changes the metrics from frankenphp_{worker.file}_worker_request_count to frankenphp_worker_request_count{name="worker.name"}, by changing the metrics name for each worker to use label, it can use the same visualization dashboard for multiple apps

[dunglas]

Couldn't we generate a name using the file path instead?

I would like to keep the public API surface as small as possible.

[withinboredom]

Currently, it just uses the path of the worker as the name. This is probably fine 99% of the time.

This is also a change I originally had in the original metrics PR (to make the metrics output prettier) -- the only real usecase here is when you have the same worker script for multiple endpoints (but even that doesn't really make sense IMHO).

I'm not really a fan of this approach though; this is much more than just adding support for names and is a pretty major refactor.

[withinboredom]

This is a good step in the right direction. A couple things:

1. it seems to hardcode prometheus as the metrics implementation.
2. it may be better to allow dynamically renaming worker names via the admin api -- this would allow decoupling devops (monitoring) from application development, which I am all for.
3. it causes a BC break.

[withinboredom]

At least until 2.0, we will need to probably support both ways for BC. Otherwise, anyone using the current metrics will have no metrics on the next release.

Ultimately, it's @dunglas's call though.

[IndraGunawan]

v1.4.0 was released last month, and since then no metrics, i didn't see anyone raise an issue (it is maybe user who utilizes metrics is in a small percentage or have not upgraded to v1.4) hence i think modifying the metrics quite safe, i know in semver it is BC break

[withinboredom]

That is a fair assessment. :)

We should probably update the documentation in this PR too.

[chrisgoldberg1]

Currently, it just uses the path of the worker as the name. This is probably fine 99% of the time.

At least to make it symlink safe: #1373 (comment)

[withinboredom]

At least to make it symlink safe

It doesn't actually read the files, so symlinks don't matter (so long as Frankenphp is consistently using the same path). It's just the only key available.

[chrisgoldberg1]

It doesn't actually read the files, so symlinks don't matter (so long as Frankenphp is consistently using the same path). It's just the only key available.

Of course I know this and there is no reason the read the files. But as you see here: #1373 (comment), there was an issue.
I specified the symlink path /proj/ that actually links to /drive1000/srv/ and for any reason it was not working. I did not go through all the code but I see that you take the path and compare it. I think that the path in vhost root directive was resolved and the workers one not, and compared with the workers one which was /proj/ vs /drive1000/srv/, it was not working.

In case I ever find time again to check it, I will deep dive into it, but something like this was the case. One of the both paths must have been resolved otherwise the comparision would match and it would have been working without any issues in first place.

[dunglas]

Maybe should we just resolve the worker path early to prevent this issue.

[chrisgoldberg1]

Maybe should we just resolve the worker path early to prevent this issue.

I just tested it again and as I received the Uncaught RuntimeException: frankenphp_handle_request() called while not in worker mode in /drive1000/srv/testproject/website/public/index.php even I specified root * /proj/testproject/website/public/, it most likely is the case that the path in the vhost is resolved and the workers one not.

So yes, resolving the worker paths from symlink to the actual one on the start must be the solution. 👍

[IndraGunawan]

the initial idea was to link Caddy metrics and FrankenPHP metrics

{
	frankenphp {
		worker {
			name service1
			file ./service1/public/index.php
		}
		worker {
			name service2
			file ./service2/public/index.php
		}
	}

	servers :8080 {
		name service1
	}

	servers :8081 {
		name service2
	}
}

:8080 {
	log
	route {
		root * ./service1/public
		php_server
	}
}


:8081 {
	log
	route {
		root * ./service2/public
		php_server
	}
}

Caddy generates metrics

caddy_http_request_duration_seconds_bucket{code="200",handler="subroute",method="GET",server="service1",le="0.005"} 6
caddy_http_request_size_bytes_bucket{code="200",handler="subroute",method="GET",server="service1",le="256"} 0
caddy_http_requests_in_flight{handler="subroute",server="service1"} 0
caddy_http_requests_total{handler="subroute",server="service1"} 6
caddy_http_response_duration_seconds_bucket{code="200",handler="subroute",method="GET",server="service1",le="0.005"} 6
caddy_http_response_size_bytes_bucket{code="200",handler="subroute",method="GET",server="service1",le="256"} 6

and FrankenPHP generates metrics

frankenphp_busy_workers{worker="service1"} 0
frankenphp_ready_workers{worker="service1"} 20
frankenphp_total_workers{worker="service1"} 20
frankenphp_worker_request_count{worker="service1"} 6
frankenphp_worker_request_time{worker="service1"} 0.0031373349999999998

so in grafana, i can select 1 label all the related metrics will be displayed.

---

it may be better to allow dynamically renaming worker names via the admin api -- this would allow decoupling devops (monitoring) from application development, which I am all for.

@withinboredom, i have limited knowledge about Caddy. for my knowledge, Can admin API change variable value or do we still need to expose the config and then admin api modify the config?
do we need to trigger the admin api every time we restart frankenphp?

[dunglas]

Ok interesting, why not then.

[withinboredom]

ok ... I'm tentatively approving this one. Let's wait until #1266 is merged and then we can see what we need to change here -- if anything.

[IndraGunawan]

this PR should be ready for review

[withinboredom]

@IndraGunawan can you rebase this to fix the conflicts?

[dunglas]

Maybe could you keep the old test as is to ensure that there is no breaking change?

[IndraGunawan]

@withinboredom has concerns about this as well #1376 (comment)

[dunglas]

Thank you!