tag1consulting / goose

Load testing framework, inspired by Locust
https://tag1.com/goose
Apache License 2.0
759 stars 69 forks source link

Refactor CSV Logging #516

Closed mTsBucy1 closed 1 year ago

mTsBucy1 commented 1 year ago

To fix #515 I added a macro that properly escapes the values. I like this approach because it is very lightweight and doesn't add any csv reader functionality as many external dependencies would. On the other hand, this still struggles with nested data and Option<> as well as hard coded headers.

However, currently in order to use the macro in the tests, I use macro_export, but then this leaks into the docs. Does anyone know how to prevent this in Edition 2018? Or is this a case for docs::hidden?

I additionally propose to flatten the GooseRequestMetric.raw into its fields method, url, headers, body for the request log and error log. For GooseDebug, which includes a whole GooseRequestMetric, a similar argument applies. The Option<> values should further be represented by an empty string when None. Since those changes potentially break external metric analysis via CSV import, I am reluctant to make these changes without further input. What do you think? Do you import goose metrics in another program? And via csv or other?

mTsBucy1 commented 1 year ago

I like this approach because it is very lightweight and doesn't add any csv reader functionality as many external dependencies would. On the other hand, this still struggles with nested data and Option<> as well as hard coded headers.

In my local testing, it fixes the bug you reported and I don't see any regressions. Where are you running into problems with nested data and Option<>?

There is no problem per se, but the existing TODO hasn't been addressed. Specifically, the csv still contains "Some(value)" and "None", instead of the more CSV-style empty string and "value".

I additionally propose to flatten the GooseRequestMetric.raw into its fields method, url, headers, body for the request log and error log. For GooseDebug, which includes a whole GooseRequestMetric, a similar argument applies.

I'm not sure I follow what you're suggesting. Are you proposing replacing the "raw" column with a column for each of the 4 contained fields? I don't see any fundamental problem with doing that.

Yes, that is exactly what I meant. It feels wrong to have json-like fields in a csv file. But that is material for a future PR.

jeremyandrews commented 1 year ago

Thanks, this is a much appreciated improvement: confirmed that CSV import works correctly now. Waiting for tests to pass then will merge.

The suggested followups would be very welcome if you wish to submit additional PRs, both expanding GooseRawRequest to proper columns, and properly formatting Option<>. ///