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.

Sign up for GitHub

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

Jump to bottom

combine storage latency and error metrics #98332

Merged
merged 1 commit into from
Jan 25, 2021
Merged

combine storage latency and error metrics #98332

merged 1 commit into from
Jan 25, 2021

Conversation

JornShen
Copy link
Member

@JornShen JornShen commented Jan 23, 2021 •
edited
Loading

What type of PR is this?

/kind feature

What this PR does / why we need it:

combine both latency and error status into one metric,
because right now, the latency metric only tracks latency
for successful operations and not error operations.

Which issue(s) this PR fixes:

ref: #98089 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

ACTION REQUIRED: Remove storage metrics `storage_operation_errors_total`, since we already have `storage_operation_status_count`.And add new field `status` for `storage_operation_duration_seconds`, so that we can know about all status storage operation latency.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 23, 2021
@JornShen
Copy link
Member Author

/cc @msau42 @mattcary @gnufied

how about this ?

@JornShen JornShen changed the title combine volumn latency and error metrics combine storage latency and error metrics Jan 23, 2021
mattcary
mattcary reviewed Jan 25, 2021
View reviewed changes
Copy link
Contributor

@mattcary mattcary left a comment

Choose a reason for hiding this comment

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

/lgtm

I'm not quite sure if it will be awkward in the future if more errors are added beyond failUnknown. But what we have here seems as good as anything.

pkg/volume/util/metrics.go
StabilityLevel: metrics.ALPHA,
},
[]string{"volume_plugin", "operation_name"},
[]string{"volume_plugin", "operation_name", "status"},
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that storageOperationEndToEndLatencyMetric suffers from the same problem, where errors don't have an e2e latency (see RecordMetrics).

But that seems to be a different sort of beast so rolling errors into that metric may not be desirable?

Copy link
Member

Choose a reason for hiding this comment

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

yeah I think we need to revisit operationendtoend in general (not in this pr). The implementation is complex, and may be better suited to be tracked by an outside observer instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

so we can merge volumeOperationErrorsMetric to storageOperationEndToEndLatencyMetric. And as same as this MR,
add a status to storageOperationEndToEndLatencyMetric ?

@mattcary
@msau42

Copy link
Member

Choose a reason for hiding this comment

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

No, the end to end metric is much more complicated and I think needs a bit of redesign. I think we should just leave it as it is for now.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2021
msau42
msau42 reviewed Jan 25, 2021
View reviewed changes
Copy link
Member

@msau42 msau42 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Thanks!

cc @gnufied @verult @kubernetes/sig-storage-pr-reviews

pkg/volume/util/metrics.go
StabilityLevel: metrics.ALPHA,
},
[]string{"volume_plugin", "operation_name"},
[]string{"volume_plugin", "operation_name", "status"},
Copy link
Member

Choose a reason for hiding this comment

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

yeah I think we need to revisit operationendtoend in general (not in this pr). The implementation is complex, and may be better suited to be tracked by an outside observer instead.

@msau42
Copy link
Member

msau42 commented Jan 25, 2021

/retest

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JornShen, msau42

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2021
@msau42
Copy link
Member

msau42 commented Jan 25, 2021

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 25, 2021
@msau42
Copy link
Member

msau42 commented Jan 25, 2021

Can you also add "ACTION REQUIRED" at the beginning of the release note since consumers of this metric may need to change?

@verult
Copy link
Contributor

verult commented Jan 25, 2021

Depending on if this PR merges first or this one: #98089, do unit tests need to be updated in one of these PRs to expect only 1 metric?

@msau42
Copy link
Member

msau42 commented Jan 25, 2021

Depending on if this PR merges first or this one: #98089, do unit tests need to be updated in one of these PRs to expect only 1 metric?

Yes, I would say merge this one first and then update the unit tests

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit bb310ac into kubernetes:master Jan 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Jan 25, 2021
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 26, 2021
@mattcary mattcary mentioned this pull request Jan 26, 2021
Closed
@msau42 msau42 mentioned this pull request Jan 28, 2021
Closed
@gnufied
Copy link
Member

gnufied commented Jan 29, 2021

Sorry for reviewing this late but we removed one metric but left storageOperationStatusMetric in place. Should't we removed that as well?

@mattcary
Copy link
Contributor

#98392 removes that metric. Somehow this change has gotten spread over three PRs :-/

@mattcary mattcary mentioned this pull request Feb 12, 2021
Merged
@dgrisonnet dgrisonnet mentioned this pull request Feb 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msau42 msau42 msau42 left review comments

@mattcary mattcary mattcary left review comments

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@saad-ali saad-ali Awaiting requested review from saad-ali

@gnufied gnufied Awaiting requested review from gnufied

Assignees

@msau42 msau42

@mattcary mattcary

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
7 participants
@JornShen @msau42 @k8s-ci-robot @verult @fejta-bot @gnufied @mattcary