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

[3.5] feat: add wal write system call metrics observation #17616

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

Akiqqqqqqq
Copy link

@Akiqqqqqqq Akiqqqqqqq commented Mar 19, 2024

xref. #17615

add new metrics: wal_write_duration_seconds

wal_write_duration_seconds represent total delay of:

syscall.write(padding_info + record_data_with_padding)

  • delay of fsync is not included in wal_write_duration_seconds
  • data: the raft log to write to the WAL on disk at one single raftNode loop

@k8s-ci-robot
Copy link

Hi @Akiqqqqqqq. 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/test-infra repository.

@Akiqqqqqqq Akiqqqqqqq force-pushed the feature-wal-write-metrics branch from 5314d83 to add5731 Compare March 19, 2024 04:09
tools/mod/go.sum Outdated Show resolved Hide resolved
@jmhbnz
Copy link
Member

jmhbnz commented Mar 19, 2024

/ok-to-test

@liangyuanpeng
Copy link
Contributor

liangyuanpeng commented Mar 19, 2024

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

Remind me if i missed something.

image

@Akiqqqqqqq Akiqqqqqqq requested a review from jmhbnz March 19, 2024 07:24
@jmhbnz
Copy link
Member

jmhbnz commented Mar 19, 2024

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

Very strange. I have not seen this issue before. Can you please rebase and force push to trigger the workflows again?

@Akiqqqqqqq
Copy link
Author

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

Very strange. I have not seen this issue before. Can you please rebase and force push to trigger the workflows again?

to be sure, so do I need to perfrom again?
git rebase HEAD~1 --signoff
and
git push -f

@Akiqqqqqqq Akiqqqqqqq force-pushed the feature-wal-write-metrics branch from 336fd8f to 79c4b75 Compare March 19, 2024 10:17
@Akiqqqqqqq
Copy link
Author

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

Very strange. I have not seen this issue before. Can you please rebase and force push to trigger the workflows again?

I retried, but seems to be not working

@jmhbnz jmhbnz closed this Mar 19, 2024
@jmhbnz jmhbnz reopened this Mar 19, 2024
@jmhbnz
Copy link
Member

jmhbnz commented Mar 19, 2024

I retried, but seems to be not working

Closing and re-opening the pr seems to have fixed it 😅

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Reading through the linked issue I think this addition makes sense, thanks @Akiqqqqqqq for drafting the pr.

Edit: Note if we do go ahead with merging this we need to update docs etc for the new metric.

server/wal/encoder.go Outdated Show resolved Hide resolved
@Akiqqqqqqq Akiqqqqqqq force-pushed the feature-wal-write-metrics branch from 79c4b75 to 13c002f Compare March 20, 2024 02:02
@Akiqqqqqqq Akiqqqqqqq closed this Mar 20, 2024
@Akiqqqqqqq Akiqqqqqqq reopened this Mar 20, 2024
@Akiqqqqqqq Akiqqqqqqq requested a review from serathius March 20, 2024 02:04
Copy link
Contributor

@liangyuanpeng liangyuanpeng left a comment

Choose a reason for hiding this comment

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

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

This caught my attention, so I looked into it a little deeper. I found out that this PR was being submitted for release-3.5 instead of main, and the release-3.5 branch didn't have the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml , so this is the reason the github acton workflow wasn't being run by approve .

I think we should create a PR to the main first?

@Akiqqqqqqq Akiqqqqqqq closed this Mar 20, 2024
@Akiqqqqqqq Akiqqqqqqq reopened this Mar 20, 2024
@Akiqqqqqqq
Copy link
Author

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

This caught my attention, so I looked into it a little deeper. I found out that this PR was being submitted for release-3.5 instead of main, and the release-3.5 branch didn't have the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml , so this is the reason the github acton workflow wasn't being run by approve .

I think we should create a PR to the main first?

ok, let me raise another PR to main

@Akiqqqqqqq
Copy link
Author

Seem like the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml have some problem here? Exist label of ok-to-test but not approve this PR to run github action.

This caught my attention, so I looked into it a little deeper. I found out that this PR was being submitted for release-3.5 instead of main, and the release-3.5 branch didn't have the workflow of https://github.com/etcd-io/etcd/blob/main/.github/workflows/gh-workflow-approve.yaml , so this is the reason the github acton workflow wasn't being run by approve .

I think we should create a PR to the main first?

check this out: #17618

@jmhbnz
Copy link
Member

jmhbnz commented Mar 20, 2024

I think we should create a PR to the main first?

Yes good catch - let's merge #17618 first.

@ivanvc
Copy link
Member

ivanvc commented Mar 21, 2024

/retitle [3.5] feat: add wal write system call metrics observation

@k8s-ci-robot k8s-ci-robot changed the title feat: add wal write system call metrics observation [3.5] feat: add wal write system call metrics observation Mar 21, 2024
@Akiqqqqqqq Akiqqqqqqq force-pushed the feature-wal-write-metrics branch from 13c002f to d9f52b2 Compare March 25, 2024 02:31
@Akiqqqqqqq Akiqqqqqqq force-pushed the feature-wal-write-metrics branch from d9f52b2 to 259951d Compare April 6, 2024 14:48
Signed-off-by: Qiuyu Wu <qiuyu.wu@shopee.com>
@ahrtr
Copy link
Member

ahrtr commented Apr 7, 2024

Please link to the original PR next time when backporting any PR,

@Akiqqqqqqq
Copy link
Author

#17615

got it

@ahrtr ahrtr merged commit b671991 into etcd-io:release-3.5 Apr 8, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants