Stackdriver / collectd

Stackdriver's monitoring agent based on collectd (http://collectd.org).
https://cloud.google.com/monitoring/agent/
Other
51 stars 15 forks source link

Dropped `name` field from API requests #171

Closed jkohen closed 4 years ago

jkohen commented 4 years ago

The prefix can only be one of "projects/", "folders/", "organizations/", but currently, probably due to a typo, it's "project/".

igorpeshansky commented 4 years ago

I think the "name" field in the JSON request is actually unused (it's taken from the URL parameter instead). So we should remove it, rather than fix the format.

jkohen commented 4 years ago

I'm not sure what caused the test failure. The same test passes locally. I'll restart the test, and if that doesn't help, I'll investigate.

I'm baffled by this, I suspect it's a glitch in Travis' cache for the code in the branch. I don't think this should block this change (which is also completely unrelated to the unit test that's failing).

The test fails with

plugin_log (3, "parse error: trailing garbage
           "RESOURCE_EXHAUSTED"    } } ���
                     (right here) ------^
");
plugin_log (3, "parse error: trailing garbage

                     (right here) ------^
");

However I inspected the file locally on the same branch, and there is no trailing garbage (and the same test passes). Moreover, the test processes the other JSON file correctly, and that file has the same trailing sequence.

~/src/collectd % od -tax1 src/time_series_summary_test.json | tail   
         20  20  20  20  20  20  20  20  20  7d  0a  20  20  20  20  20
0002140  sp   ]   ,  nl  sp  sp  sp  sp  sp  sp   "   s   t   a   t   u
         20  5d  2c  0a  20  20  20  20  20  20  22  73  74  61  74  75
0002160   s   "  sp   :  sp   "   R   E   S   O   U   R   C   E   _   E
         73  22  20  3a  20  22  52  45  53  4f  55  52  43  45  5f  45
0002200   X   H   A   U   S   T   E   D   "  nl  sp  sp  sp   }  nl   }
         58  48  41  55  53  54  45  44  22  0a  20  20  20  7d  0a  7d
0002220  nl
         0a
0002221
~/src/collectd % od -tax1 src/collectd_time_series_response_test.json | tail
         20  20  7d  0a  20  20  20  20  20  20  5d  2c  0a  20  20  20
0002440  sp  sp  sp   "   t   o   t   a   l   P   o   i   n   t   C   o
         20  20  20  22  74  6f  74  61  6c  50  6f  69  6e  74  43  6f
0002460   u   n   t   "  sp   :  sp   4   ,  nl  sp  sp  sp  sp  sp  sp
         75  6e  74  22  20  3a  20  34  2c  0a  20  20  20  20  20  20
0002500   "   s   u   c   c   e   s   s   P   o   i   n   t   C   o   u
         22  73  75  63  63  65  73  73  50  6f  69  6e  74  43  6f  75
0002520   n   t   "  sp   :  sp   1  nl  sp  sp  sp   }  nl   }  nl
         6e  74  22  20  3a  20  31  0a  20  20  20  7d  0a  7d  0a
0002537
igorpeshansky commented 4 years ago

Explaining the test failure: this is a classic buffer overrun problem. read_file_contents, which is used by the test, does not add a trailing '\0'. Locally, you happen to have a '\0' character in your buffer after the file contents (with, perhaps, some ignored whitespace), but on Travis, the buffer has other random contents that isn't that. One way to handle this would be to memset(buf, '\0', sizeof(buf)) before calling read_file_contents. Another is to add a '\0' explicitly into the buffer after reading the file (see https://github.com/Stackdriver/collectd/blob/stackdriver-agent-5.8.1/src/processes.c#L1369-L1373). Side note: you need to leave enough space for that trailing '\0' — other examples use sizeof(buf)-1 for that.

Given that read_file_contents is only used to read text files, from what I can see, perhaps the right solution is to send a PR upstream renaming it to read_text_file_contents and ensure that it adds the trailing '\0' itself, and cherry-pick that commit into our fork…

You're right that the failure is unrelated to this PR, however.