Skip to content

request-benchmark: require --content-type in informer mode - #4365

Merged
kubernetes-prow[bot] merged 4 commits into
kubernetes:masterfrom
p0lyn0mial:informer-default-json-content-type
Sep 21, 2026
Merged

kubernetes-prow[bot] merged 4 commits into
kubernetes:masterfrom
p0lyn0mial:informer-default-json-content-type

Conversation

@p0lyn0mial

@p0lyn0mial p0lyn0mial commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

make the content-type default to "json" instead of "proto" for the informer mode, matching the http mode.

the only job that runs informer mode sets the content type explicitly to proto.

this PR allows me to also test kubernetes/test-infra#37854

@kubernetes-prow kubernetes-prow Bot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 11, 2026
@kubernetes-prow kubernetes-prow Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 11, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/assign @serathius

apiVersion := fs.String("api-version", "v1", "apiVersion of the target resource (e.g. v1, apps/v1).")
resource := fs.String("resource", "secrets", "resource name of the target resource (e.g. pods, deployments).")
contentType := fs.String("content-type", "proto", "Content type for informer requests. Valid values: [json, proto]")
contentType := fs.String("content-type", "json", "Content type for informer requests. Valid values: [json, proto]")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One ask (I know it's nitpicking), I prefer we didn't have a default. I would prefer that for things like content-type the user needs to explicitly specify so you don't need to read code to confirm what is the default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it make sense. i will adjust the pr.

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from fde7c7b to de56bac Compare September 11, 2026 11:15
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

/hold

until the CI jobs above pass.

@kubernetes-prow kubernetes-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 11, 2026
@p0lyn0mial p0lyn0mial changed the title request-benchmark: default informer content-type to json request-benchmark: require --content-type in informer mode Sep 11, 2026
@kubernetes-prow kubernetes-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 11, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

// Re-create stopCh so that a second start/gather cycle can Dispose
// cleanly. Dispose closes stopCh, so reusing the one from a previous
// cycle (or the constructor) would panic with "close of closed channel".
e.stopCh = make(chan struct{})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah, just testing :) i don't want to merge it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ack, recommend testing locally on kind. It's faster but up to you.

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from b58dfe4 to 2e46f0d Compare September 13, 2026 10:08
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

1 similar comment
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

testing #4376

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

testing kubernetes/test-infra#37866

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from dc3b0cc to de56bac Compare September 14, 2026 16:43
@kubernetes-prow kubernetes-prow Bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 14, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

I need to bump the number of pods based on:

pull-perf-tests-benchmark-list-informer:
    {
      "data": {
        "Perc50": 300,
        "Perc90": 380,
        "Perc99": 398
      },
      "unit": "ms",
      "labels": {
        "Count": "4038",
        "Resource": "pods",
        "Scope": "cluster",
        "SlowCount": "0",
        "Subresource": "",
        "Verb": "LIST"
      }
    },

and

pull-perf-tests-benchmark-list-proto:
   {
      "data": {
        "Perc50": 300,
        "Perc90": 380,
        "Perc99": 398
      },
      "unit": "ms",
      "labels": {
        "Count": "11422",
        "Resource": "pods",
        "Scope": "cluster",
        "SlowCount": "0",
        "Subresource": "",
        "Verb": "LIST"
      }
    },

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

testing with kubernetes/test-infra#37875

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

testing with kubernetes/test-infra#37880

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@kubernetes-prow kubernetes-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 17, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from f89cc6d to 27460f6 Compare September 18, 2026 16:13
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from 27460f6 to d2de0db Compare September 18, 2026 19:23
@kubernetes-prow kubernetes-prow Bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from d2de0db to 3f21b07 Compare September 19, 2026 10:06
@kubernetes-prow kubernetes-prow Bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 19, 2026
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial
p0lyn0mial force-pushed the informer-default-json-content-type branch from 3f21b07 to 7a14415 Compare September 20, 2026 08:44
@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/test pull-perf-tests-benchmark-list-informer
/test pull-perf-tests-benchmark-list-proto

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

informer mode:

    {
      "data": {
        "Perc50": 4516.129032,
        "Perc90": 4929.032258,
        "Perc99": 5680
      },
      "unit": "ms",
      "labels": {
        "Count": "260",
        "Resource": "pods",
        "Scope": "cluster",
        "SlowCount": "0",
        "Subresource": "",
        "Verb": "LIST"
      }
    },

http mode:

    {
     "data": {
       "Perc50": 4583.02583,
       "Perc90": 5301.123595,
       "Perc99": 5940.224719
     },
     "unit": "ms",
     "labels": {
       "Count": "643",
       "Resource": "pods",
       "Scope": "cluster",
       "SlowCount": "0",
       "Subresource": "",
       "Verb": "LIST"
     }
   },

To understand the difference in the number of requests between the two modes, we would have to download the logs from the client binary and maybe even add some instrumentation.

@p0lyn0mial

Copy link
Copy Markdown
Contributor Author

/hold cancel

@kubernetes-prow kubernetes-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2026
@serathius

Copy link
Copy Markdown
Member

/lgtm
/approve

@kubernetes-prow kubernetes-prow Bot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2026
@kubernetes-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: p0lyn0mial, serathius

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

The pull request process is described here

Details 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

@kubernetes-prow kubernetes-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2026
@kubernetes-prow
kubernetes-prow Bot merged commit 2a436df into kubernetes:master Sep 21, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants