Enable Vault Prometheus #454

ashu3103 · 2024-07-24T12:11:51Z

Enable Prometheus in vault

About this commit

Bind an HTTP server in pgagroal-vault at port metrics in configuration to track server activities
Added some fields like - metrics, hugepage, metric_cache_max_age and metric_cache_max_size in the common configuration
Added an additional check is_prometheus_enabled on all the prometheus functions and code blocks which basically checks whether the metrics port is active or not.
Based on the metric port value we allocate memory to prometheus structure and prometheus_cache

Future work

Segregate the prometheus structure for main and vault similar to configuration for optimal memory management.
prometheus should be initiated only when metric port is active --> Document it properly somewhere.

@jesperpedersen @fluca1978 PTAL.

jesperpedersen · 2024-07-24T12:34:42Z

src/libpgagroal/prometheus.c


 void
 pgagroal_prometheus(int client_fd)
 {
+   if (!is_prometheus_enabled())


You should do this after the variable definitions

jesperpedersen · 2024-07-24T12:34:53Z

src/libpgagroal/prometheus.c

+void
+pgagroal_vault_prometheus(int client_fd)
+{
+   if (!is_prometheus_enabled())


jesperpedersen · 2024-07-24T12:35:14Z

src/libpgagroal/prometheus.c

@@ -244,6 +312,11 @@ pgagroal_init_prometheus(size_t* p_size, void** p_shmem)
 void
 pgagroal_prometheus_session_time(double time)
 {
+   if (!is_prometheus_enabled())


jesperpedersen · 2024-07-24T12:35:19Z

src/libpgagroal/prometheus.c

@@ -330,6 +403,11 @@ pgagroal_prometheus_session_time(double time)
 void
 pgagroal_prometheus_connection_error(void)
 {
+   if (!is_prometheus_enabled())


jesperpedersen · 2024-07-24T12:35:26Z

src/libpgagroal/prometheus.c

@@ -340,6 +418,11 @@ pgagroal_prometheus_connection_error(void)
 void
 pgagroal_prometheus_connection_kill(void)
 {
+   if (!is_prometheus_enabled())


jesperpedersen · 2024-07-24T12:39:54Z

src/libpgagroal/prometheus.c

+   data = pgagroal_append(data, "  </p>\n");
+   data = pgagroal_append(data, "  <h2>pgagroal_self_sockets</h2>\n");
+   data = pgagroal_append(data, "  <p>\n");
+   data = pgagroal_append(data, "   Number of sockets used by pgagroal itself\n");


pgagroal-vault

jesperpedersen · 2024-07-24T12:40:14Z

src/libpgagroal/prometheus.c

@@ -503,6 +656,11 @@ pgagroal_prometheus_auth_user_error(void)
 void
 pgagroal_prometheus_client_wait_add(void)
 {
+   if (!is_prometheus_enabled())


Well, all of them...

jesperpedersen · 2024-07-24T12:41:23Z

src/libpgagroal/prometheus.c

+   data = pgagroal_append(data, "  <p>\n");
+   data = pgagroal_append(data, "   <a href=\"/metrics\">Metrics</a>\n");
+   data = pgagroal_append(data, "  </p>\n");
+   data = pgagroal_append(data, "  <h2>pgagroal_logging_info</h2>\n");


These should be called pgagroal_vault_XYZ

jesperpedersen · 2024-07-24T12:41:56Z

src/libpgagroal/prometheus.c

+
+   prometheus = (struct prometheus*)prometheus_shmem;
+
+   data = pgagroal_append(data, "#HELP pgagroal_logging_info The number of INFO logging statements\n");


pgagroal_vault_logging_info

jesperpedersen · 2024-07-24T12:42:21Z

src/libpgagroal/prometheus.c

+
+   data = pgagroal_append(data, "#HELP pgagroal_client_sockets Number of sockets the client used\n");
+   data = pgagroal_append(data, "#TYPE pgagroal_client_sockets gauge\n");
+   data = pgagroal_append(data, "pgagroal_client_sockets ");


pgagroal_vault_XYZ

jesperpedersen · 2024-07-24T12:45:35Z

Remember the tutorial, man page and manual as well

ashu3103 · 2024-07-24T12:47:14Z

Remember the tutorial, man page and manual as well

Yeah working on it parallely

jesperpedersen · 2024-07-24T12:48:40Z

Maybe enable metrics by default in the pgagroal_vault.conf as well

jesperpedersen · 2024-07-24T12:59:03Z

I think that your "Future" section should be easy as well

jesperpedersen · 2024-07-24T13:12:38Z

prometheus should be initiated only when metric port is active --> Document it properly somewhere.

It is documented in the configuration files - users doesn't need to know about the internals

ashu3103 · 2024-07-24T13:14:10Z

prometheus should be initiated only when metric port is active --> Document it properly somewhere.

It is documented in the configuration files - users doesn't need to know about the internals

Yeah, but for new developers it might be required

jesperpedersen · 2024-07-24T13:28:04Z

You can put it in

ashu3103 · 2024-07-25T18:27:59Z

@jesperpedersen The suggested changes have been integrated PTAL.

fluca1978 · 2024-07-26T09:21:07Z

src/libpgagroal/prometheus.c

@@ -2267,6 +2824,7 @@ pgagroal_init_prometheus_cache(size_t* p_size, void** p_shmem)

 error:
   // disable caching
+


please remove the empty line and leave the comment near the code

fluca1978 · 2024-07-26T09:39:03Z

src/libpgagroal/prometheus.c

+static bool
+is_prometheus_enabled(void)
+{
+   struct configuration* config = (struct configuration*)shmem;


add test against nulls?

Not required as we initiate prometheus memory allocation only if config->metrics > 0.

Not required as we initiate prometheus memory allocation only if config->metrics > 0.

Well, but if we export the function to somewhere else, we could need some extra (redundant) checks.

Okay, will add the NULL check for prometheus to avoid any unlikely cases that may occur.

fluca1978 · 2024-07-26T09:40:26Z

src/main.c

@@ -742,20 +742,23 @@ main(int argc, char** argv)
      errx(1, "Failed to start logging");
   }

-   if (pgagroal_init_prometheus(&prometheus_shmem_size, &prometheus_shmem))
+   if (config->common.metrics > 0)


should we export is_prometheus_active function and use it so the code is consistent?

Yeah can be done, but won't it waste some memory and computation as config is already there in the program it will again define struct configuration in the check function.

However, as far as consistency is concerned it can be done.

Yeah can be done, but won't it waste some memory and computation as config is already there in the program it will again define struct configuration in the check function.

The only waste I can see is the function call, but I don't think we are optimizing at this level right now.
It could also be a macro.

The only waste I can see is the function call, but I don't think we are optimizing at this level right now.

Okay 👍

It could also be a macro.

Not sure how a macro could solve the issue, can you please elaborate a little.

Also, if we add NULL check of prometheus in is_prometheus_enabled() function (as per the aforementioned change request) then we can't use it in this scenario as it will always fail the test (since prometheus is checked before it is being initialised), and the only test which should be checked is metrics > 0. So I believe the current setup is ok.

What do you think about it? @fluca1978.

Also, if we add NULL check of prometheus in is_prometheus_enabled() function (as per the aforementioned change request) then we can't use it in this scenario as it will always fail the test (since prometheus is checked before it is being initialised), and the only test which should be checked is metrics > 0. So I believe the current setup is ok.

Indeed having the test against nulls will fail in this case, so better leave it off the is_prometheus_enabled. Sorry, I was running out of coffee.

fluca1978 · 2024-07-26T09:40:52Z

src/main.c

@@ -1044,14 +1047,14 @@ main(int argc, char** argv)
      ev_periodic_start (main_loop, &rotate_frontend_password);
   }

-   if (config->metrics > 0)
+   if (config->common.metrics > 0)


same as above

fluca1978 · 2024-07-26T09:42:36Z

src/vault.c

@@ -379,6 +435,25 @@ main(int argc, char** argv)
      errx(1, "Failed to start logging");
   }

+   if (config->common.metrics > 0)


same as above

fluca1978 · 2024-07-26T09:42:54Z

src/vault.c

@@ -445,21 +529,64 @@ main(int argc, char** argv)

   start_vault_io();

+   if (config->common.metrics > 0)


same as above

fluca1978 · 2024-07-26T09:51:39Z

src/include/pgagroal.h

+/** @struct
+ * Defines the Main Prometheus metrics
+ */
+struct main_prometheus


I don't get the point in splitting the prometheus structure into two parts, it seems to me it is useless: if metrics > 0 prometheus is active, so there is no advantage in memory allocation. Also, while vault_configuration added fields around configuration, here you are narrowing the structure only to the fields you need.
While I don't dislike in total this approach, it seems counterintuitive to have prometheus not being the main metrics structure as I would at glange expect. I would rather make a prometheus_logging structure, a prometheus_socket one and have prometheus to wrap the above, so that prometheus_vault can handle only the former twos.

if metrics > 0 prometheus is active, so there is no advantage in memory allocation.

True, the aim of splitting prometheus is to separate the memory space allocation for different programs (main/vault) as there as several fields which are not utilized by vault but are still allocated extra memory for unused fields.

Also, while vault_configuration added fields around configuration, here you are narrowing the structure only to the
fields you need.

As of now I have added just the required fields and more fields can be added as and when required.

While I don't dislike in total this approach, it seems counterintuitive to have prometheus not being the main metrics
structure as I would at glange expect. I would rather make a prometheus_logging structure, a prometheus_socket
one and have prometheus to wrap the above, so that prometheus_vault can handle only the former twos.

Can be done that way also. The thing i kept in my mind was that if in future if the project integrates other such service programs like vault, that will need prometheus then it can directly inherit the common fields (common for all services/servers) while keep on adding its specific fields in its prometheus structure.

Can be done that way also. The thing i kept in my mind was that if in future if the project integrates other such service programs like vault, that will need prometheus then it can directly inherit the common fields (common for all services/servers) while keep on adding its specific fields in its prometheus structure.

This is clear, and is a good approach, but I don't think that common is the right name for the structure. It seems, so far, you need only a subset of the structure for the vault, so I'm not sure if this effort in splitting the structure is worth. I suspect you will find the need to add more fields here and there from time to time, so at this stage I would not split the structure. Surely, I will not name it as common, since that makes me think at common fields, while so far there are only "basic" fields like logging counters.
@jesperpedersen what do you think?

prometheus_base is an alternative

jesperpedersen · 2024-07-29T13:40:18Z

src/include/pgagroal.h

+/** @struct
+ * Defines the Main Prometheus metrics
+ */
+struct main_prometheus


prometheus_base is an alternative

jesperpedersen · 2024-07-29T13:43:09Z

I think it looks good but prometheus_common or prometheus_base is probably better as a name

jesperpedersen · 2024-07-30T17:55:12Z

Merged.

Thanks for your contribution !

jesperpedersen requested review from jesperpedersen and fluca1978 July 24, 2024 12:18

jesperpedersen assigned ashu3103 Jul 24, 2024

jesperpedersen added the feature New feature label Jul 24, 2024

jesperpedersen requested changes Jul 24, 2024

View reviewed changes

ashu3103 force-pushed the prometheus-active branch from 6bebd7a to 67ecc0d Compare July 25, 2024 18:25

fluca1978 requested changes Jul 26, 2024

View reviewed changes

jesperpedersen requested changes Jul 29, 2024

View reviewed changes

Enable Vault Prometheus

bb3253a

ashu3103 force-pushed the prometheus-active branch from 67ecc0d to bb3253a Compare July 30, 2024 04:30

jesperpedersen merged commit bb3253a into agroal:master Jul 30, 2024
2 checks passed


		prometheus = (struct prometheus*)prometheus_shmem;

		data = pgagroal_append(data, "#HELP pgagroal_logging_info The number of INFO logging statements\n");

		@@ -2267,6 +2824,7 @@ pgagroal_init_prometheus_cache(size_t* p_size, void** p_shmem)

		error:
		// disable caching

		@@ -445,21 +529,64 @@ main(int argc, char** argv)

		start_vault_io();

		if (config->common.metrics > 0)

Enable Vault Prometheus #454

Enable Vault Prometheus #454

Conversation

ashu3103 commented Jul 24, 2024 • edited Loading

Enable Prometheus in vault

About this commit

Future work

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesperpedersen commented Jul 24, 2024

ashu3103 commented Jul 24, 2024

jesperpedersen commented Jul 24, 2024

jesperpedersen commented Jul 24, 2024

jesperpedersen commented Jul 24, 2024

ashu3103 commented Jul 24, 2024

jesperpedersen commented Jul 24, 2024

ashu3103 commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesperpedersen commented Jul 29, 2024

jesperpedersen commented Jul 30, 2024

ashu3103 commented Jul 24, 2024 •

edited

Loading