search

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

Dynamically allocate memory when doing curl requests in write_gcm. #114

Closed bmoyles0117 closed 7 years ago

bmoyles0117 commented 7 years ago

PTAL

jkohen commented 7 years ago

The conversation is interesting, but I think you're stretching what's basically a hack. It won't take much more effort to fix the bug by not failing on a large response, and doing heap allocation, for which we even have (presumably) working code.

bmoyles0117 commented 7 years ago

Changed up the approach to use dynamic allocation, PTAL.

bmoyles0117 commented 7 years ago

Feedback addressed, PTAL.

kosak commented 7 years ago

BY THE WAY (and apologies for parachuting into this thread in the first place), but can I ask what this change is for anyway? As far as I can tell, the only thing that is done with large responses is (a) interpret them as an error, and then (b) write them to the log (which will probably truncate them to some sane size anyway). Is sucking down the whole response text useful for some feature?

dhrupadb commented 7 years ago

@kosak We have enabled partial success in the monitoring API which allows some points to be ingested and the API to return a meaningful response regarding ones that weren't. The response code for a partial success error is however, a 200. In order not to truncate the response as it's likely to have meaningful debugging info -- we need to ensure the buffer correctly accommodates it.

jkohen commented 7 years ago

Corey, thanks for taking a step back and listing the priorities. I agree with that list, but I think we got sidetracked from the top priority: Make the code path that writes points work when the response doesn't fit the buffer, because that code only uses the response for debug logging. We should fix that ASAP, as it's a customer visible issue.