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

metrics: seperate metrics with source scope for txn command #723

Merged
merged 3 commits into from
Mar 6, 2023

Conversation

cfzjywxk
Copy link
Contributor

@cfzjywxk cfzjywxk commented Mar 2, 2023

ref: pingcap/tidb#41203

Add source scope for several metrics.

The grafana change result would be posted latter, related PR would be opened in the tidb repo.

Copy link
Contributor

@you06 you06 left a comment

Choose a reason for hiding this comment

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

And may change the implementation of KVTxn.isInternal and KVSnapshot.IsInternal to inherit IsInternal from RequestSource.

func (r *RequestSource) IsInternal() bool {
	if r == nil {
		return false
	}
	return r.RequestSourceInternal
}

TxnRegionsNumHistogramPessimisticLock prometheus.Observer
TxnRegionsNumHistogramPessimisticRollbackInternal prometheus.Observer
TxnRegionsNumHistogramPessimisticRollback prometheus.Observer
TxnRegionsNumHistogramWithCoprocessor prometheus.Observer
Copy link
Contributor

Choose a reason for hiding this comment

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

TxnRegionsNumHistogramWithCoprocessorInternal is missing

TxnRegionsNumHistogramPessimisticLock = TiKVTxnRegionsNumHistogram.WithLabelValues("2pc_pessimistic_lock", LblGeneral)
TxnRegionsNumHistogramPessimisticRollbackInternal = TiKVTxnRegionsNumHistogram.WithLabelValues("2pc_pessimistic_rollback", LblInternal)
TxnRegionsNumHistogramPessimisticRollback = TiKVTxnRegionsNumHistogram.WithLabelValues("2pc_pessimistic_rollback", LblGeneral)
TxnRegionsNumHistogramWithBatchCoprocessorInternal = TiKVTxnRegionsNumHistogram.WithLabelValues("coprocessor", LblInternal)
Copy link
Contributor

Choose a reason for hiding this comment

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

The type "coprocessor" is wrong, and the init of TxnRegionsNumHistogramWithCoprocessor and TxnRegionsNumHistogramWithCoprocessorInternal is missing.

metrics.TiKVTxnWriteKVCountHistogram.Observe(float64(commitDetail.WriteKeys))
metrics.TiKVTxnWriteSizeHistogram.Observe(float64(commitDetail.WriteSize))

isInternalReq := util.IsInternalRequest(c.txn.GetRequestSource())
Copy link
Contributor

@you06 you06 Mar 2, 2023

Choose a reason for hiding this comment

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

Suggested change
isInternalReq := util.IsInternalRequest(c.txn.GetRequestSource())
isInternalReq := c.txn.isInternal()

TxnRegionsNumHistogramWithBatchCoprocessorInternal = TiKVTxnRegionsNumHistogram.WithLabelValues("coprocessor", LblInternal)
TxnRegionsNumHistogramWithBatchCoprocessor = TiKVTxnRegionsNumHistogram.WithLabelValues("batch_coprocessor", LblGeneral)
TxnWriteKVCountHistogramInternal = TiKVTxnWriteKVCountHistogram.WithLabelValues(LblInternal)
TxnWriteKVCountHistogramGeneral = TiKVTxnWriteKVCountHistogram.WithLabelValues(LblGeneral)
Copy link
Contributor

@you06 you06 Mar 2, 2023

Choose a reason for hiding this comment

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

TxnWriteKVCountHistogramInternal and TxnWriteKVCountHistogramGeneral are not initialized.

Unluckily there is no codegen by macro for such works :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missing TxnWriteSizeHistogramInternal and TxnWriteSizeHistogramGeneral are added.

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Mar 2, 2023

And may change the implementation of KVTxn.isInternal and KVSnapshot.IsInternal to inherit IsInternal from RequestSource.

func (r *RequestSource) IsInternal() bool {
	if r == nil {
		return false
	}
	return r.RequestSourceInternal
}

@you06
There's a method wrapped in https://github.com/tikv/client-go/blob/master/util/request_source.go#L64, I think it's better to check nil first and void using nil references.

@you06
Copy link
Contributor

you06 commented Mar 2, 2023

@

There's a method wrapped in https://github.com/tikv/client-go/blob/master/util/request_source.go#L64, I think it's better to check nil first and void using nil references.

Yes the nil check is required, what I mean is to avoid making string and check it, we can check RequestSource.RequestSourceInternal directly if possible. The function IsInternalRequest is used when we have request context, the request source is formatted as a string in it.

@cfzjywxk
Copy link
Contributor Author

cfzjywxk commented Mar 3, 2023

@you06
Got it, I've changed the util.IsRequestSourceInternal implementation to use the boolean RequestSourceInternal directly.

Signed-off-by: cfzjywxk <lsswxrxr@163.com>
Signed-off-by: cfzjywxk <lsswxrxr@163.com>
Signed-off-by: cfzjywxk <lsswxrxr@163.com>
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