etcd

etcd copied to clipboard

This PR is intended to address this issue: https://github.com/etcd-io/etcd/issues/16866

Hi @thedtripp. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Hi @thedtripp, it looks like your commit message is wrongly formatted (signed-off-by appears twice). I also suggest co-authoring with the author of the original PR #16902.

/ok-to-test

Hi @jmhbnz, the original suggestion from https://github.com/etcd-io/etcd/pull/16902#pullrequestreview-1725849250 was to add an integration or e2e case for this functionality. But later in https://github.com/etcd-io/etcd/issues/16866#issuecomment-2104180883, you refer to it as basic testing. The current implementation introduced unit tests. Is this what we're looking for?

Hi @thedtripp, it looks like your commit message is wrongly formatted (signed-off-by appears twice). I also suggest co-authoring with the author of the original PR #16902.

/ok-to-test

@ivanvc. Fixed. Thanks!

Hi @jmhbnz, the original suggestion from #16902 (review) was to add an integration or e2e case for this functionality. But later in #16866 (comment), you refer to it as basic testing. The current implementation introduced unit tests. Is this what we're looking for?

The unit test is a good start, I think ideally we should expand tests/integration/metrics_test.go or tests/e2e/metrics_test.go to have coverage for this new metric. @thedtripp please take a look at implementing an integration or e2e test and let us know if you have any questions, thanks.

Hi @jmhbnz, the original suggestion from #16902 (review) was to add an integration or e2e case for this functionality. But later in #16866 (comment), you refer to it as basic testing. The current implementation introduced unit tests. Is this what we're looking for?

The unit test is a good start, I think ideally we should expand tests/integration/metrics_test.go or tests/e2e/metrics_test.go to have coverage for this new metric. @thedtripp please take a look at implementing an integration or e2e test and let us know if you have any questions, thanks.

@jmhbnz Is there any test coverage for the existing transaction metrics (applySec and slowApplies)? I'm trying to figure out how to write the requested integration or e2e test. Thanks.

Commit https://github.com/etcd-io/etcd/pull/17983/commits/4f029a9fc232c69afe1d3d11cfbf5a111ab31e2c, which set threshold to 300 seconds, resulted in failing tests. I will increase the value and try again.

Thanks, @thedtripp, this is looking good. I suggest squashing your commits into a single one in preparation for merging this PR.

Thanks, @thedtripp, this is looking good. I suggest squashing your commits into a single one in preparation for merging this PR.

Is it ok to squash the commits with interactive rebase?

Is it ok to squash the commits with interactive rebase?

Yes! That's how I usually do it :roll_eyes: :smile:

Thanks @thedtripp for your first PR.

Overall looks good to me. Please also add a changelog item in CHANGELOG-3.6.md#metrics-monitoring