[collectd 6] Initial implementation for epics plugin #4282

matwey · 2024-02-18T18:46:38Z

EPICS is Experimental Physics and Industrial Control System.

ChangeLog: epics plugin: Initial implementation for epics plugin for collectd 6.0

eero-t

IMHO it would be better to have v5 version in the first commit, and apply v6 changes on top of that in second commit. As v5 plugin is already in, it would mean that one needs to review only the v5->v6 changes.

octo

Hi @matwey, thanks for the PR! I left some comments inline, the most severe is probably the casting and dereferencing of pointers, which I think is very likely to cause problems.

octo · 2024-02-20T08:06:51Z

src/epics.c

+  bool thread_loop;
+} epics_plugin = {.lock = PTHREAD_MUTEX_INITIALIZER};
+
+static void free_pvs() {


Please use this instead:

static void free_pvs(void) { // ^^^^

The difference is that with () the compiler does not check the (unused) arguments being passed to the function. This is backwards compatibility behavior to way back when and should not be used.

octo · 2024-02-20T16:04:33Z

src/epics.c

+
+    if (p->is_label) {
+      free(p->label);
+    } else if (p->metric_list.ptr) {


free() handles NULL gracefully. This allows us to simplify this code to execute the non-label code unconditionally.

octo · 2024-02-20T16:06:16Z

src/epics.c

+  struct pv *pvs;
+  size_t pvs_num;
+
+  metric_family_t *mfs;


Please use families for global variables and struct fields and fams for local variables, instead of mfs. By consistently using the same name for common structures the codebase becomes much more readable.

octo · 2024-02-20T16:10:49Z

src/epics.c

+}
+
+static int epics_config_metric_family(oconfig_item_t *ci, size_t mf_index) {
+  metric_family_t *fam = &epics_plugin.mfs[mf_index];


It's easier to just pass metric_family_t *fam as an argument.

mf_index is required to call epics_config_metric and epics_config_label. I decided to store family index instead of pointer in order to allow mfs reallocations. Otherwise, dangling pointers fix would be required on each mfs reallocation.

src/epics.c

octo · 2024-02-20T16:35:52Z

src/epics.c

+  if (metric_type == METRIC_TYPE_COUNTER &&
+      (ch_type == DBR_SHORT || ch_type == DBR_LONG)) {
+    return DBR_LONG;
+  } else if (metric_type == METRIC_TYPE_GAUGE &&
+             (ch_type == DBR_FLOAT || ch_type == DBR_DOUBLE ||
+              ch_type == DBR_ENUM)) {
+    return DBR_DOUBLE;
+  }


Casting and then dereferencing a short* to a long*, or a float* to a double* will probably not work.

I think we could create two functions, one returning the value as an integer and the other returning the value as a double, e.g.:

static int64_t as_int64(evargs args) { switch (args.type) { case DBR_SHORT: return *((short const *) args.dbr); case DBR_LONG: return *((long const *) args.dbr); case DBR_DOUBLE: return *((double const *) args.dbr); case DBR_FLOAT: return *((float const *) args.dbr); // not sure what to do about DBR_ENUM } return 0; } static double as_double(evargs args) { // … }

Then call one or the other, depending on the metric type:

static value_t as_value(evargs args, metric_type_t type) { switch (type) { case METRIC_TYPE_COUNTER: return (value_t){.counter = as_int64(args)}; case METRIC_TYPE_GAUGE: return (value_t){.gauge = as_double(args)}; // … } ERROR("epics plugin: invalid metric type: %d", type); return (value_t){0}; }

Finally, in the event handling you can then simply do this:

metric_list->ptr[i].value = as_value(args, epics_plugin.mfs[p->mf_index].type);

I decided to simplify overall logic here. As far as I understand, there are two different types in Epics:

DBF (Field type) is a type of the remote field,

DBR (Request type) is a type requested from the remote side.
The library performs conversions from actual DBF to requested DBR under the hood, including the conversions such as parsing from DBF_STRING to DBR_DOUBLE for instance.

So, as soon as we provide DBR_DOUBLE or DBR_LONG to ca_create_subscription as first argument, we will see only DBR_DOUBLE or DBR_LONG in handle_var_event and then dereferencing is safe.

The code you mentioned should be

if (metric_type == METRIC_TYPE_COUNTER && (ch_type == **DBF**_SHORT || ch_type == **DBF**_LONG)) { return DBR_LONG; } else if (metric_type == METRIC_TYPE_GAUGE && (ch_type == **DBF**_FLOAT || ch_type == **DBF**_DOUBLE || ch_type == **DBF**_ENUM)) { return DBR_DOUBLE; }

Fortunately, DBF_SHORT and DBR_SHORT shares the same integer code. The code tries to notify the user that he wants to use unusual combination of DBR/DBF type, however, now I think it is for user to know what he wants.

octo · 2024-02-20T16:46:49Z

src/epics.c

+  epics_plugin.pvs_num = 0;
+}
+
+static void free_mfs() {


-> free_families

Explicit empty argument list: (void)

octo · 2024-02-20T16:55:01Z

src/epics.c

+    for (struct pv *p = &epics_plugin.pvs[pv_i];
+         p != epics_plugin.pvs + epics_plugin.pvs_num && p->mf_index == i;
+         ++p) {


This is very hard to read. I think this would be much simpler:

// rename "pv_i" to "offset" for (size_t j = 0; j < epics_plugin.pvs_num; j++) { size_t index = offset + j; struct pv *p = &epics_plugin.pvs[index]; if (p->mf_index != i || !p->is_active || !p->is_label) { continue; } // … }

octo · 2024-02-26T20:16:44Z

I tried building the plugin in the CI system, but without success:

A bug in the Debian package prevents the configure script from finding the EPICS package: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1059706
Apparently libvirt introduced an out-of-bounds write that our unit tests pick up, causing "Debian unstalbe" to fail. This is unrelated to this PR though.

octo · 2024-02-26T20:20:54Z

@matwey Looks like this is progressing nicely. Let me know when the PR is ready for a review :D

matwey · 2024-02-28T09:06:48Z

@matwey Looks like this is progressing nicely. Let me know when the PR is ready for a review :D

I am slowly progressing. Now I've found that metric_list_t is supposed to be a list of the same metric values at different time moments. I don't know why I wrongly decided this was purposed for vector-valued metrics. So I need extra time to redesign the plugin.

eero-t · 2024-02-28T09:36:56Z

src/epics.c

+  union {
+    metric_list_t metric_list;
+    char *label;
+  };


metric_list is array of pointers, label is a single pointer, i.e. they have different allocation and free handling.

By stuffing them to same union you save 8 bytes per pv struct. What's the max number of pv structs you expect there to be?

(Union should be named, so people reading code notice more clearly that its members overlap.)

eero-t · 2024-02-28T09:44:36Z

src/epics.c

+      free(p->metric_list.ptr);
+      p->metric_list.num = 0;


This is array of structs containing pointers to additional lists (labels, maybe also metadata). Freeing just the root array leaks the rest. => Use the appropriate metric functions instead.

eero-t · 2024-02-28T09:46:58Z

src/epics.c

+  for (size_t i = 0; i < epics_plugin.families_num; ++i) {
+    metric_family_t *fam = &epics_plugin.families[i];
+
+    free(fam->name);
+    free(fam->help);
+    free(fam->unit);
+    metric_family_metric_reset(fam);
+  }
+
+  free(epics_plugin.families);


Don't duplicate existing functionality, use the corresponding function from metric.h instead.

metric_family_free cannot be applied here because it tries to free individual fam pointer which is incorrect.

Designing the code so that it uses existing helper functions to allocate/update/free given objects, makes its maintenance easier. Only those helpers need to be updated; one does not need search, understand, update and test also all the other places that do their things slightly differently from the helpers...

I am totally agree, but there is no suitable existing helper function, what do you suggest exactly?

Allocate pointers individually, so that they can be separately freed?

eero-t · 2024-02-28T09:51:52Z

src/epics.c

+  }
+
+  p->family_index = family_index;
+  p->is_label = 1;


This is a bool:

Suggested change

p->is_label = 1;

p->is_label = true;

eero-t · 2024-02-28T09:54:14Z

src/epics.c

+  return pthread_join(epics_plugin.thread_id, NULL);
+}
+
+static int epics_config_metric(oconfig_item_t *ci, struct pv *p,


This is not returning any error code, just -1 or 0, so return value could as well be a bool.

Same applies also to the other functions following this.

eero-t · 2024-02-28T10:00:10Z

src/epics.c

+    return -1;
+  }
+
+  p->family_index = family_index;


For clarity and to be more symmetric with the other function:

Suggested change

p->family_index = family_index;

p->family_index = family_index;

p->is_label = false;

eero-t · 2024-02-28T10:11:23Z

src/epics.c

+
+    label_set_reset(&label_set);
+    for (struct pv *p = &epics_plugin.pvs[offset];
+         p != epics_plugin.pvs + epics_plugin.pvs_num; ++p) {


Just in case:

Suggested change

p != epics_plugin.pvs + epics_plugin.pvs_num; ++p) {

p < epics_plugin.pvs + epics_plugin.pvs_num; ++p) {

eero-t · 2024-02-28T10:12:20Z

src/epics.c

+    }
+
+    for (struct pv *p = &epics_plugin.pvs[offset];
+         p != epics_plugin.pvs + epics_plugin.pvs_num; ++p, ++offset) {


Just in case:

Suggested change

p != epics_plugin.pvs + epics_plugin.pvs_num; ++p, ++offset) {

p < epics_plugin.pvs + epics_plugin.pvs_num; ++p, ++offset) {

eero-t · 2024-02-28T10:17:03Z

src/epics.c

+static int epics_config_metric_family(oconfig_item_t *ci, size_t family_index) {
+  metric_family_t *fam = &epics_plugin.families[family_index];


Any particular reason for passing and (storing to pv struct) an index to a global array instead of a metric_family_t pointer?

Yes, families are reallocated at each config function invocation. Storing the pointer would easily make it dangling.

eero-t · 2024-02-28T10:28:09Z

src/epics.c

@@ -0,0 +1,575 @@
+/**
+ * collectd - src/epics.c
+ * Copyright (C) 2022       Matwey V. Kornilov


eero-t · 2024-02-28T10:42:43Z

Now I've found that metric_list_t is supposed to be a list of the same metric values at different time moments.

It's array of new metrics for given metric family (family = metric type with specific name, unit, and resource attributes). You submit them as part of family (after which you reset i.e. free them), so they will have the same timestamp.

You use metric labels to differentiate different metric values within given metric family.

For example, in the Sysman plugin, for GPU frequency there are two metric families, frequency and frequency ratio (of max possible). Both of these families have separate metric values for frequency request (from SW) and actual frequency (set on HW by FW).

OpenTelemetry specs have large number of examples for metric (family) names, and their (metric) labels: https://opentelemetry.io/docs/specs/semconv/

EPICS is Experimental Physics and Industrial Control System. Reference: https://epics-controls.org/ Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>

collectd-bot added this to the 6.0 milestone Feb 18, 2024

eero-t reviewed Feb 19, 2024

View reviewed changes

octo added Feature New plugin labels Feb 20, 2024

octo requested changes Feb 20, 2024

View reviewed changes

matwey force-pushed the epics-6.0 branch 3 times, most recently from e2a2d99 to 17b6a76 Compare February 25, 2024 20:12

matwey force-pushed the epics-6.0 branch 2 times, most recently from cf6eff7 to e6daf11 Compare February 28, 2024 08:39

eero-t reviewed Feb 28, 2024

View reviewed changes

eero-t changed the title ~~Initial implementation for epics plugin for collectd 6.0~~ [collectd 6] Initial implementation for epics plugin Feb 28, 2024

matwey force-pushed the epics-6.0 branch 2 times, most recently from 08c77ec to f7f4eb6 Compare March 6, 2024 12:13

Initial implementation for epics plugin for collectd 6.0

81ef16a

EPICS is Experimental Physics and Industrial Control System. Reference: https://epics-controls.org/ Signed-off-by: Matwey V. Kornilov <matwey.kornilov@gmail.com>

matwey force-pushed the epics-6.0 branch from f7f4eb6 to 81ef16a Compare March 6, 2024 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[collectd 6] Initial implementation for epics plugin #4282

[collectd 6] Initial implementation for epics plugin #4282

matwey commented Feb 18, 2024 •

edited

eero-t left a comment

octo left a comment

octo Feb 20, 2024

octo Feb 20, 2024

octo Feb 20, 2024

octo Feb 20, 2024

matwey Feb 22, 2024

octo Feb 20, 2024

matwey Feb 25, 2024 •

edited

matwey Feb 25, 2024

octo Feb 20, 2024

octo Feb 20, 2024

octo commented Feb 26, 2024

octo commented Feb 26, 2024

matwey commented Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

matwey Mar 6, 2024

eero-t Mar 6, 2024 •

edited

matwey Mar 6, 2024

eero-t Mar 6, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

eero-t Feb 28, 2024

matwey Mar 6, 2024

eero-t Feb 28, 2024

eero-t commented Feb 28, 2024

	p->family_index = family_index;
	p->family_index = family_index;
	p->is_label = false;

	p != epics_plugin.pvs + epics_plugin.pvs_num; ++p) {
	p < epics_plugin.pvs + epics_plugin.pvs_num; ++p) {

		static int epics_config_metric_family(oconfig_item_t *ci, size_t family_index) {
		metric_family_t *fam = &epics_plugin.families[family_index];

[collectd 6] Initial implementation for epics plugin #4282

Are you sure you want to change the base?

[collectd 6] Initial implementation for epics plugin #4282

Conversation

matwey commented Feb 18, 2024 • edited

eero-t left a comment

Choose a reason for hiding this comment

octo left a 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

matwey Feb 25, 2024 • edited

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

octo commented Feb 26, 2024

octo commented Feb 26, 2024

matwey commented Feb 28, 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

eero-t Mar 6, 2024 • edited

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

eero-t commented Feb 28, 2024

matwey commented Feb 18, 2024 •

edited

matwey Feb 25, 2024 •

edited

eero-t Mar 6, 2024 •

edited