Allow storing test metrics in local filesystem of LocalStack container#13188
Allow storing test metrics in local filesystem of LocalStack container#13188nik-localstack merged 1 commit intomainfrom
Conversation
nik-localstack
added
semver: patch
Test Results (amd64) - Integration, Bootstrap 5 files 5 suites 2h 38m 49s ⏱️ Results for commit ecdd453. ♻️ This comment has been updated with latest results. |
alexrashed
added
the
notes: skip
nik-localstack
force-pushed
the
k8s/tests-api-coverage
branch
from
316165c to
d3b685b
Compare
nik-localstack
force-pushed
the
k8s/tests-api-coverage
branch
3 times, most recently
from
578366d to
1fda6e1
Compare
nik-localstack
force-pushed
the
k8s/tests-api-coverage
branch
from
1fda6e1 to
ecdd453
Compare
cloutierMat
left a comment
cloutierMat
left a comment
There was a problem hiding this comment.
Thanks for tackling this! 🚀 Overall the changes look good and I like the use of constants here instead of hardcoded strings 👍
I am not sure about the file saving strategy, we now lose the de-duplicate and we are doing file operations on every request. I would like have a bit more insight over the impact of those changes before merging.
| # refrain from adding duplicates | ||
| if metric not in MetricHandler.metric_data: | ||
| MetricHandler.metric_data.append(metric) | ||
| self.append_metric(metric) |
There was a problem hiding this comment.
question: Since when storing the file locally, we don't append to MetricHandler.metric_data this won't prevent duplicate. Do you know if the risk of duplicate entry is high? The pr that introduces it doesn't give much more information.
There was a problem hiding this comment.
I don't understand the need of adding duplicates. In the localstack-docs script https://github.com/localstack/localstack-docs/blob/main/scripts/create_data_coverage.py that uses these files, duplication is not affecting the results.
There was a problem hiding this comment.
I am not sure either, it is also only called from a single response handler, not too sure how there could even be a duplicate in the first place 🤷♂️
| with open(self.local_filename, "a") as fd: | ||
| writer = csv.writer(fd) | ||
| writer.writerow(metric) |
There was a problem hiding this comment.
question: My understanding is that every request going through our gateway will create an entry and open/close the csv file. Have you tested the cost implication of this? Do you have any full run example showing the additional cost is minimal?
There was a problem hiding this comment.
The tests may take ~3 minutes more than before so the impact is not that big.
We can reiterate and improve later if it starts becoming an issue, wdyt ?
nik-localstack
left a comment
nik-localstack
left a comment
There was a problem hiding this comment.
Thanks for the review @cloutierMat
I agree with your concerns, and I tried to think of a better approach but given that this flow is a bit outdated (and most of the data that are created from these report files are not used anymore) I don't think it makes sense to invest more time now. Ideally we can re-iterate if this becomes an issue, or if/when we clean up this process a bit.
| with open(self.local_filename, "a") as fd: | ||
| writer = csv.writer(fd) | ||
| writer.writerow(metric) |
There was a problem hiding this comment.
The tests may take ~3 minutes more than before so the impact is not that big.
We can reiterate and improve later if it starts becoming an issue, wdyt ?
| # refrain from adding duplicates | ||
| if metric not in MetricHandler.metric_data: | ||
| MetricHandler.metric_data.append(metric) | ||
| self.append_metric(metric) |
There was a problem hiding this comment.
I don't understand the need of adding duplicates. In the localstack-docs script https://github.com/localstack/localstack-docs/blob/main/scripts/create_data_coverage.py that uses these files, duplication is not affecting the results.
cloutierMat
left a comment
cloutierMat
left a comment
There was a problem hiding this comment.
Thanks for answering my concerns @nik-localstack!
🚀 LGTM
| # refrain from adding duplicates | ||
| if metric not in MetricHandler.metric_data: | ||
| MetricHandler.metric_data.append(metric) | ||
| self.append_metric(metric) |
There was a problem hiding this comment.
I am not sure either, it is also only called from a single response handler, not too sure how there could even be a duplicate in the first place 🤷♂️
| with open(self.local_filename, "a") as fd: | ||
| writer = csv.writer(fd) | ||
| writer.writerow(metric) |
3 participants
Motivation
We want to collect test metrics when the system running pytest is not the same running LocalStack
Changes
ENV_INTERNAL_TEST_STORE_METRICS_IN_LOCALSTACKenvironment variable to enable local filesystem storageENV_INTERNAL_TEST_STORE_METRICS_PATHenvironment variable to specify custom storage path (defaults to/tmp/localstack-metrics)