Initial support for gathering metrics from multiple nginx instances. #4125
Conversation
src/nginx.c
Outdated
| /* Local arrays for temporal storing of the parameters available through | ||
| * plugin options */ | ||
|
|
||
| char u[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char p[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char url[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char verify_peer[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char verify_host[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char cacert[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char timeout[DATA_MAX_NAME_LEN] = {'\0'}; | ||
| char socket[DATA_MAX_NAME_LEN] = {'\0'}; |
There was a problem hiding this comment.
This function is nearly two hundred lines. This could be good place to split it (at least) into two.
Single char variables should be used only as loop counters (i, j etc), anything else should have more descriptive names (especially in function this large).
socket overrides standard C-lib function name. Maybe "sockname"?
Otherwise this looks pretty OK on quick glance.
There was a problem hiding this comment.
Thank you for review and for your notes @eero-t .
Definitely you are right about nginx_add_daemon_config() function size and not very obvious names for temporary arrays. So in order to solve the second issue instead of using predefined number of static arrays I decided to return back static char *config_keys[] setting list and use dynamic **parsed_values_for_nginx_instance string list for parsing configuration file and further curl options settings. Additionally in terms of the first issue curl options were splitted into default ones ( that cannot be changed from configuration file ) and optional ones that are managed with config.
…string list for getting per-instance settings from collectd configuration file.
|
|
||
| /* A temporary list of strings where per nginx instance settings from collectd | ||
| config are stored before passing the to curl an index of setting in this list | ||
| corellates to the one from config_keys list, e.g. |
There was a problem hiding this comment.
corellates -> correlates
And do not move code (config_keys*) around, it makes things harder to review.
| parsed_values_for_nginx_instance = | ||
| (char **)calloc(config_keys_num, sizeof(char *)); | ||
| for (int i = 0; i < config_keys_num; i++) { | ||
| parsed_values_for_nginx_instance[i] = | ||
| (char *)calloc(DATA_MAX_NAME_LEN, sizeof(char)); | ||
| } |
There was a problem hiding this comment.
You have deinit function for freeing the allocs, so there should be also corresponding init function doing the allocs (close together).
| /* Zeroing per nginx instance temporary stringlist for getting settings from | ||
| * collectd config for each nginx instance */ | ||
| static void init_nginx_instance_settings_array(void) { | ||
| for (int i = 0; i < config_keys_num; i++) { | ||
| for (int j = 0; j < DATA_MAX_NAME_LEN; j++) { | ||
| parsed_values_for_nginx_instance[i][j] = '\0'; | ||
| } | ||
| } | ||
|
|
||
| if ((*var = strdup(value)) == NULL) | ||
| return 1; | ||
| else | ||
| return 0; | ||
| } | ||
|
|
||
| static int config(const char *key, const char *value) { | ||
| if (strcasecmp(key, "url") == 0) | ||
| return config_set(&url, value); | ||
| else if (strcasecmp(key, "user") == 0) | ||
| return config_set(&user, value); | ||
| else if (strcasecmp(key, "password") == 0) | ||
| return config_set(&pass, value); | ||
| else if (strcasecmp(key, "verifypeer") == 0) | ||
| return config_set(&verify_peer, value); | ||
| else if (strcasecmp(key, "verifyhost") == 0) | ||
| return config_set(&verify_host, value); | ||
| else if (strcasecmp(key, "cacert") == 0) | ||
| return config_set(&cacert, value); | ||
| else if (strcasecmp(key, "timeout") == 0) | ||
| return config_set(&timeout, value); | ||
| else if (strcasecmp(key, "socket") == 0) | ||
| return config_set(&sock, value); | ||
| else | ||
| return -1; | ||
| } /* int config */ | ||
|
|
||
| static int init(void) { | ||
| if (curl != NULL) | ||
| curl_easy_cleanup(curl); | ||
|
|
||
| if ((curl = curl_easy_init()) == NULL) { | ||
| ERROR("nginx plugin: curl_easy_init failed."); | ||
| return -1; | ||
| /******************/ | ||
|
|
||
| /* Cleanup per nginx instance temporary stringlist for storing settings from | ||
| * collectd config for each nginx instance */ | ||
| static void deinit_nginx_instance_settings_array(void) { | ||
| for (int i = 0; i < config_keys_num; i++) { | ||
| if (parsed_values_for_nginx_instance[i] != NULL) { | ||
| sfree(parsed_values_for_nginx_instance[i]); | ||
| parsed_values_for_nginx_instance[i] = NULL; | ||
| } |
There was a problem hiding this comment.
While using an array makes certain things easier, I do not really like how you did it. You have no names for the indexes, they're just magic values, when you use them later. I.e. it's really annoying to check whether they e.g. match.
I'd rather have parsed_values_for_nginx_instance a struct, with named members matching to config keys, so you can refer to them by name in set_optional_curl_params().
Helper functions doing init + deinit for those struct members, could have auxiliary array though, like this:
static const char *nginx_inst_str_ptrs[config_keys_num] = { &parsed_values_for_nginx_instance.username, ... };
(Such helper could also include the names of the keys, so you can use it in parsing too.)
| /******* Nginx plugin config parsing. Inspired by ceph plugin config parser | ||
| * implementation ( see ceph.c ) *******/ | ||
| static int nginx_handle_str(struct oconfig_item_s *item, char *dest, | ||
| int dest_len) { | ||
| const char *val; | ||
| if (item->values_num != 1) { | ||
| return -ENOTSUP; | ||
| } | ||
| if (item->values[0].type != OCONFIG_TYPE_STRING) { | ||
| return -ENOTSUP; | ||
| } | ||
| val = item->values[0].value.string; | ||
| if (snprintf(dest, dest_len, "%s", val) > (dest_len - 1)) { | ||
| ERROR("nginx plugin: configuration parameter '%s' is too long.\n", | ||
| item->key); | ||
| return -ENAMETOOLONG; | ||
| } | ||
| return 0; | ||
| } /* void init */ | ||
| } |
There was a problem hiding this comment.
Earlier code did not use strings with fixed lengths, but took the strings from config as-is in config_set(). I think the new code should do the same.
ChangeLog: nginx plugin: Initial support for gathering metrics from multiple nginx instances. see #780
Several days ago I faced with a case in which I needed to gather /status metrics from multiple nginx instances with a single collectd instance, something like #780.
I created a patch to the current nginx plugin implementation that covers this above mentioned case. The main idea of the patch is to create a number of structures that store configured "InstanceName" and a CURL object containing options for curl-ing of each nginx instance separately. The config parser implementation was inspired by the one for ceph plugin. The only difference between them is a static limitation on a total number of nginx instances which can be served by plugin simultaneously. It's defined with a constant value:
As a consequence of changing of config parser method, a configuration format for plugin was changed as well, according to contrib/nginx/nginx-multiple-instances.conf