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

istio: add metric for debouncing #40523

Merged
merged 3 commits into from
Aug 22, 2022
Merged

istio: add metric for debouncing #40523

merged 3 commits into from
Aug 22, 2022

Conversation

yingzhuivy
Copy link
Member

@yingzhuivy yingzhuivy commented Aug 17, 2022 •
edited by istio-policy-bot
Loading

Please provide a description of this PR:
This CL adds metric for the delay between a first config change
enters deboucing until the final merged push request is pushed into
the push queue. This time plus the proxy convergence time give us
an upper bound on the total delay between a config change and the
change is pushed to proxies.

Also increased the buckets since logging shows that the debounce time
is pretty long (more than 1 minute).

{"level":"info","time":"2022-08-05T21:55:46.153204Z","scope":"ads","msg":"Push debounce stable[2555] 1193 for config ServiceEntry/balboa-production/balboa-production.balboa-production.svc.ea1.us.a and 182 more configs: 92.851119ms since last change, 1m9.085819569s since last push, full=true"}

istio: add metric for debouncing
2379230 
This CL adds metric for the delay between a first config change
enters deboucing until the final merged push request is pushed into
the push queue. This time plus the proxy convergence time give us
an upper bound on the total delay between a config change and the
change is pushed to proxies.

Also increased the buckets since logging shows that the debounce time
is pretty long (more than 1 minute).

Change-Id: I3220f9c3188824ea6925151ff6837f91aac5a15a
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3512
Reviewed-by: Weibo He <weibo.he@airbnb.com>
Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>
Reviewed-by: Jungho Ahn <jungho.ahn@airbnb.com>
@yingzhuivy yingzhuivy requested review from a team as code owners August 17, 2022 17:26
@istio-policy-bot istio-policy-bot added the release-notes-none Indicates a PR that does not require release notes. label Aug 17, 2022
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 17, 2022
istio: fix debounceTime typo
3f701a8 
Change-Id: I9977c597768360cc3dd485dbf21bd9afdb2f5151
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3517
Reviewed-by: Weibo He <weibo.he@airbnb.com>
howardjohn
howardjohn reviewed Aug 17, 2022
View reviewed changes
pilot/pkg/xds/monitoring.go Outdated Show resolved Hide resolved
pilot/pkg/xds/ads.go Outdated Show resolved Hide resolved
istio: handle debounce time entirely in the debounce() function
1e55b06 
To address comments in istio#40523,
we will record after push finishes inside the debounce function.
We can actually remove the DebounceStart field in the push context
as a result.

Also change the buckets as discussed in the PR.

Change-Id: I3b7a7860590e7e5ed4f13282b4398527de089c81
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3519
Reviewed-by: Jungho Ahn <jungho.ahn@airbnb.com>
Reviewed-by: Weibo He <weibo.he@airbnb.com>
@yingzhuivy
Copy link
Member Author

@howardjohn please take another look, thanks!

@istio-testing istio-testing merged commit c9557ae into istio:master Aug 22, 2022
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Aug 23, 2022
@ryan-smick
istio: add metric for debouncing (istio#40523)
9149fba 
This CL patches commit c9557ae from
upstream istio into air-release-1.14.3 to add a metric for pilot
debounce time. Original PR can be found here:
istio#40523.
The description from that PR is reproduced below.

This CL adds metric for the delay between a first config change
enters deboucing until the final merged push request is pushed into
the push queue. This time plus the proxy convergence time give us
an upper bound on the total delay between a config change and the
change is pushed to proxies.

Also increased the buckets since logging shows that the debounce time
is pretty long (more than 1 minute).

Change-Id: If6cbdd7ee9fd5aed94001a3857989bada1919c87
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3559
Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>
Reviewed-by: Douglas Jordan <douglas.jordan@airbnb.com>
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Sep 13, 2022
@ryan-smick
istio: apply patches to 1.14.4
dd5afbd 
Apply the following list of patches to istio 1.14.4:
* sidecar: filter service ports to VS ports (istio#39067)
* istio: register init push context metric (istio#40049)
* istio: add metric for debouncing (istio#40523)
* istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719)
* istio: support inline multi-values header in authz header match
(https://gerrit.musta.ch/c/public/istio/+/3622,
not yet merged upstream)

Change-Id: I69b861d884608dabad44db1fee03f66eb4b25ab2
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3695
Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>
Reviewed-by: Weibo He <weibo.he@airbnb.com>
yingzhuivy pushed a commit to airbnb/istio that referenced this pull request Oct 6, 2022
istio: add metric for debouncing (istio#40523)
d02b7d5 
* istio: add metric for debouncing

This CL adds metric for the delay between a first config change
enters deboucing until the final merged push request is pushed into
the push queue. This time plus the proxy convergence time give us
an upper bound on the total delay between a config change and the
change is pushed to proxies.

Also increased the buckets since logging shows that the debounce time
is pretty long (more than 1 minute).

Change-Id: I3220f9c3188824ea6925151ff6837f91aac5a15a
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3512
Reviewed-by: Weibo He <weibo.he@airbnb.com>
Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>
Reviewed-by: Jungho Ahn <jungho.ahn@airbnb.com>

* istio: fix debounceTime typo

Change-Id: I9977c597768360cc3dd485dbf21bd9afdb2f5151
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3517
Reviewed-by: Weibo He <weibo.he@airbnb.com>

* istio: handle debounce time entirely in the debounce() function

To address comments in istio#40523,
we will record after push finishes inside the debounce function.
We can actually remove the DebounceStart field in the push context
as a result.

Also change the buckets as discussed in the PR.

Change-Id: I3b7a7860590e7e5ed4f13282b4398527de089c81
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3519
Reviewed-by: Jungho Ahn <jungho.ahn@airbnb.com>
Reviewed-by: Weibo He <weibo.he@airbnb.com>
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Oct 12, 2022
@ryan-smick
istio: apply patches to 1.14.5
bbfd7e4 
Apply the following list of patches to istio 1.14.5:
* sidecar: filter service ports to VS ports (istio#39067)
* istio: register init push context metric (istio#40049)
* istio: add metric for debouncing (istio#40523)
* istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719)
* istio: support inline multi-values header in authz header match
(https://gerrit.musta.ch/c/public/istio/+/3622,
not yet merged upstream)
* istio: improve deep copy for ServiceAttribute (istio#40966)
* avoid unnecessary copy virtual services for sidecar scope calculation (istio#41101)

Change-Id: Ia4c9bfd619a0eb38c1a829bff2efbd21fd3b9cb2
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3883
Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>
Reviewed-by: Weibo He <weibo.he@airbnb.com>
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Nov 10, 2022
@dwj300
istio: apply patches to 1.15.3
3b2521a 
Apply the following list of upstream commits to istio 1.15.3:
* istio: add metric for debouncing (istio#40523)
* istio: fix PILOT_ENABLE_RDS_CACHE flag not working (istio#40719)
* istio: improve deep copy for ServiceAttribute (istio#40966)
* avoid unnecessary copy virtual services for sidecar scope calculation
  (istio#41101)

Change-Id: I2ee1d77d096a329dc8f590151223b37193dd4f1b
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3990
Reviewed-by: Ying Zhu <ying.zhu@airbnb.com>
Reviewed-by: Ryan Smick <ryan.smick@airbnb.com>
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Nov 10, 2022
istio: increase debounce bucket
10cb496 
Our current debounce time is a little above 30s which is outside of
the current bound. Even though community only wants bucket up to
30s (istio#40523 (comment)),
we should still have more buckets for our use.

I don't worry about performance and cost since there isn't any tags on
the metric.

Change-Id: I34b1d787936ea3d8f0a0e16259c06d4bfef8e793
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3992
Reviewed-by: Douglas Jordan <douglas.jordan@airbnb.com>
@S-Chan
Copy link
Contributor

S-Chan commented Jan 4, 2023

/cherrypick release-1.15

@istio-testing istio-testing mentioned this pull request Jan 4, 2023
Closed
@istio-testing
Copy link
Collaborator

@S-Chan: new pull request created: #42668

In response to this:

/cherrypick release-1.15

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.

@istio-testing
Copy link
Collaborator

@S-Chan: new pull request could not be created: failed to create pull request against istio/istio#release-1.15 from head istio-testing:cherry-pick-40523-to-release-1.15: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for istio-testing:cherry-pick-40523-to-release-1.15."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherrypick release-1.15

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.

istio-testing pushed a commit to istio-testing/istio that referenced this pull request Jan 4, 2023
@istio-testing
istio: handle debounce time entirely in the debounce() function
832738c 
To address comments in istio#40523,
we will record after push finishes inside the debounce function.
We can actually remove the DebounceStart field in the push context
as a result.

Also change the buckets as discussed in the PR.

Change-Id: I3b7a7860590e7e5ed4f13282b4398527de089c81
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/3519
Reviewed-by: Jungho Ahn <jungho.ahn@airbnb.com>
Reviewed-by: Weibo He <weibo.he@airbnb.com>
airbnb-gerrit pushed a commit to airbnb/istio that referenced this pull request Feb 10, 2023
@dwj300
istio: apply patches to 1.15.5
4274a85 
Apply the following list of upstream commits to istio 1.15.5:
* istio: add metric for debouncing (istio#40523)
* istio: improve deep copy for ServiceAttribute (istio#40966)
* avoid unnecessary copy virtual services for sidecar scope calculation
  (istio#41101)

Change-Id: I25f31e5633b77982606912bcb2ad2bc4e2da87f4
Reviewed-on: https://gerrit.musta.ch/c/public/istio/+/4381
Reviewed-by: Weibo He <weibo.he@airbnb.com>
Reviewed-by: Stephen Chan <stephen.chan@airbnb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yingzhuivy @S-Chan @istio-testing @howardjohn @istio-policy-bot