Hi @baomingwang. Thanks for your PR.

I'm waiting for a kubernetes 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.

/assign @deads2k @lavalamp
cc @jingyih @micahhausler who was working on previous PR

/triage accepted
/cc @jpbetz @jingyih
/sig instrumentation

/ok-to-test

/priority important-soon

should we have a test that flips a bit in etcd, then calls storage accessor methods and ensures this metric increments?

also, we decode in paths other than get and list (on every write, in watch, etc). should this metric get incremented if decode errors on existing data are encountered in those methods?

should we have a test that flips a bit in etcd, then calls storage accessor methods and ensures this metric increments?

I did some local testing to have targeting metric storage_decode_errors emitted by replace protoEncodingPrefix from []byte{0x6b, 0x38, 0x73, 0x00} to []byte{0x6b, 0x38, 0x73, 0x01}.

$ kubectl get pods nginx
Error from server: illegal base64 data at input

$ kubectl get --raw /metrics | grep storage_decode_errors
# HELP apiserver_storage_decode_errors_total [ALPHA] Number of stored object decode errors split by object type
# TYPE apiserver_storage_decode_errors_total counter
apiserver_storage_decode_errors_total{resource="pods"} 1

I1208 07:56:17.397972 10 store.go:181] Decoding pods "/pods/default/nginx" failed: illegal base64 data at input byte 3

Is it enough?

also, we decode in paths other than get and list (on every write, in watch, etc). should this metric get incremented if decode errors on existing data are encountered in those methods?

Really nice callout. You remind me that it would be better to capture decode error inside of decode() function, to capture every codec.Decode errors. Does it make sense to you?
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go#L998-L1001

@liggitt

Comments addressed with updated commit. Can you pls take a look again?
Let me know if anything didn't look to you.
@liggitt @dims

/lgtm

will wait for @liggitt to remove hold!

LGTM label has been added.

/retest

/lgtm
/approve
/hold cancel

LGTM label has been added.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baomingwang, dims, liggitt, logicalhan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

• The PR does have any do-not-merge/* labels
• The PR does not have the needs-ok-to-test label
• The PR is mergeable (does not have a needs-rebase label)
• The PR is approved (has cncf-cla: yes, lgtm, approved labels)
• The PR is failing tests required for merge

You can:

• Review the full test history for this PR
• Prevent this bot from retesting with /lgtm cancel or /hold
• Help make our tests less flaky by following our Flaky Tests Guide

/retest