Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storage/txnrecovery: add transaction recovery metrics #37807

Merged

Conversation

nvanbenschoten
Copy link
Member

This commit adds a series of time series metrics that provide insight
into the process of transaction recovery due to abandoned parallel
commits.

New metrics:

  • txnrecovery.attempts.pending
  • txnrecovery.attempts.total
  • txnrecovery.successes.committed
  • txnrecovery.successes.aborted
  • txnrecovery.successes.pending
  • txnrecovery.failures

Release note: None

@nvanbenschoten nvanbenschoten requested review from ajwerner and a team May 24, 2019 21:33
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit adds a series of time series metrics that provide insight
into the process of transaction recovery due to abandoned parallel
commits.

New metrics:
- `txnrecovery.attempts.pending`
- `txnrecovery.attempts.total`
- `txnrecovery.successes.committed`
- `txnrecovery.successes.aborted`
- `txnrecovery.successes.pending`
- `txnrecovery.failures`

Release note: None
The function used to contain more logic, but now it just defers to
IsParallelCommit, so there's no point in keeping it.

Release note: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/txnRecoveryMetrics branch from 1d2a776 to c5a760a Compare May 24, 2019 22:01
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, 1 of 1 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/storage/txnrecovery/manager_test.go, line 53 at r1 (raw file):

}

type metricVals struct {

This pattern is handy but verbose. It’d be nice to automate the generation of these from a metrics struct. Could be a good little Friday project.

Copy link
Member Author

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/storage/txnrecovery/manager_test.go, line 53 at r1 (raw file):

Previously, ajwerner wrote…

This pattern is handy but verbose. It’d be nice to automate the generation of these from a metrics struct. Could be a good little Friday project.

Yeah, this seems like a cool idea. Code gen might be a bit of a big hammer though. I bet we could do something a little more lightweight with reflection and a map[string]int64. Let's keep this in mind for the next time we turn to this pattern.

craig bot pushed a commit that referenced this pull request May 26, 2019
37807: storage/txnrecovery: add transaction recovery metrics r=nvanbenschoten a=nvanbenschoten

This commit adds a series of time series metrics that provide insight
into the process of transaction recovery due to abandoned parallel
commits.

New metrics:
- `txnrecovery.attempts.pending`
- `txnrecovery.attempts.total`
- `txnrecovery.successes.committed`
- `txnrecovery.successes.aborted`
- `txnrecovery.successes.pending`
- `txnrecovery.failures`

Release note: None

Co-authored-by: Nathan VanBenschoten <[email protected]>
@craig
Copy link
Contributor

craig bot commented May 26, 2019

Build succeeded

@craig craig bot merged commit c5a760a into cockroachdb:master May 26, 2019
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/txnRecoveryMetrics branch June 3, 2019 23:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants