Skip to content

Conversation

@bukka
Copy link
Member

@bukka bukka commented Jul 20, 2021

This PR removes start time which didn't really makes sense and renames some metrics for openmetrics status format to be inline with https://github.com/hipages/php-fpm_exporter#metrics-collected .

@bukka bukka force-pushed the fpm_openmetrics_status_sync branch from 9fdbabf to 590af46 Compare July 21, 2021 18:59
@bukka bukka merged commit 590af46 into php:master Jul 21, 2021
Comment on lines -413 to +412
"# HELP phpfpm_max_listen_queue_total The maximum number of requests in the queue of pending connections since FPM has started.\n"
"# TYPE phpfpm_max_listen_queue_total counter\n"
"phpfpm_max_listen_queue_total %d\n"
"# HELP phpfpm_max_listen_queue The maximum number of requests in the queue of pending connections since FPM has started.\n"
"# TYPE phpfpm_max_listen_queue counter\n"
"phpfpm_max_listen_queue %d\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @bukka I get the reasoning for this change, the reason I went for these names instead of matching them with that exporter is to match the Prometheus naming conversion: https://prometheus.io/docs/practices/naming/

The main question that is going around in my mind right now, is do we want to match up with that exporter, or set our own naming following the Prometheus naming convention?

Copy link
Member Author

@bukka bukka Jul 22, 2021

Choose a reason for hiding this comment

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

Well you agreed about the sync: #5723 (comment) so I did it that way. I think there's some value to have a compatibility mode with the exporter as it makes the migration pretty easy. Although after reading the conventions I feel the right way would be to follow them. Maybe we should later introduce some kind of versioning with a format using correct conventions and labels. Now it's too late to change anything as beta1 has been just released.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well you agreed about the sync: #5723 (comment) so I did it that way.

Correct, and I'm on the fence about it now. Some of them actually make sense to me, like phpfpm_max_active_processes, but might not follow that convention.

I think there's some value to have a compatibility mode with the exporter as it makes the migration pretty easy. Although after reading the conventions I feel the right way would be to follow them. Maybe we should later introduce some kind of versioning with a format using correct conventions and labels.

How would versioning work? An additional query field to version that?

Now it's too late to change anything as beta1 has been just released.

shrug it's not a very big deal, just think it would be nice if we did it perfectly 😎 . Having this out there is a massive win already 🎉 .

Copy link
Member Author

@bukka bukka Jul 25, 2021

Choose a reason for hiding this comment

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

How would versioning work? An additional query field to version that?

I was thinking about making it more query based with the same keys and different values and not differentiated by keys. For example something like format=openmetrics&version=2.

Copy link
Member Author

Choose a reason for hiding this comment

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

We would of course keep the old queries working for quite some time so it doesn't break between the versions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would work, assuming that will come in PHP 8.2 then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah hopefully :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Well then let me pot an reminder for that in my calendar :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants