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

proposal: Prombench Custom Benchmarks #41

Merged
merged 4 commits into from
Dec 20, 2024
Merged

proposal: Prombench Custom Benchmarks #41

merged 4 commits into from
Dec 20, 2024

Conversation

bwplotka
Copy link
Member

@bwplotka bwplotka commented Dec 4, 2024

Proposes implementation for prometheus/test-infra#659

Signed-off-by: bwplotka <bwplotka@gmail.com>
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Thank you for this. I agree in general, this seems like a good idea. Made some comments.

Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

One thing I liked very much about this proposal is its easy and fast implementation! The lack of custom benchmarks blocks a lot of work, so having something going quickly is nice.

When we discussed this in the past, I had something different in mind inspired by how Prow parses comments to add extra functionality to plugins.

The idea is to improve comment-monitor with new commands:

/prombench main
/extra-args --enable-features=native-histograms,wal-records --web.enable-otlp-receiver
/with avalanche
/with agent-mode
  • extra-args is a special command that adds extra command line flags to Prometheus; it can be used to add feature flags or "regular" commands
  • with is used for more complex modifications, like replacing/updating the load generator or features that require changing several parts of Prometheus. For example, agent mode requires an extra flag and removing parts of the config file.

This proposal is a lot harder though, not sure how much time that would take 😬


The main downside of the current proposal is that maintaining several branches for each kind of benchmarks seems like a lot of work. If we make a change in the main branch that should be added to all others, the number of PRs would get out of control 😬

@bwplotka
Copy link
Member Author

bwplotka commented Dec 5, 2024

Nice @ArthurSens - one of my alternatives is similar to that, so essentially "prombench options for every variation" - where you have some kind of custom config passed via /prombench or comment in general. I will add your syntax too to this proposal doc.

This proposal is a lot harder though, not sure how much time that would take 😬

It's a good question, changing comment-monitor is easy, applying those options further down (and validating) it's bit harder but I like branching a bit more, because it unlocks almost all customizations vs carefully curating prombench options and debating which ones to support, which one you have (you have to document those) etc.

Also, my proposal and yours are not conflicting. We could implement branching so we can experiment and do more complex variations, but since it's harder to maintain those branches over time, we could implement common "variation" options in the prombench long term (so your more proposal). WDYT?

cc @bboreham @krajorama @jesusvazquez @codesome any preferences on "branching" vs "prombech options" so far?

@bwplotka
Copy link
Member Author

bwplotka commented Dec 5, 2024

There is also a bit more "maintainable" path where instead of branches we do custom directories on main and you can specify different directory than default one. Then we can even use kustomize to create variations or just copy - bit easier to discover what's possible and maintain 🤔 Will add as a 3rd option.

bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
Implements first phase of prometheus/proposals#41

Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
See prometheus/proposals#41 for rationale.

Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
See prometheus/proposals#41 for rationale.

Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka added a commit to prometheus/prometheus that referenced this pull request Dec 17, 2024
bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
…` flags.

See prometheus/proposals#41 for rationale.

Prometheus job got updated with prometheus/prometheus#15682

Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
…` flags.

See prometheus/proposals#41 for rationale.

Prometheus job got updated with prometheus/prometheus#15682

Signed-off-by: bwplotka <bwplotka@gmail.com>
bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 17, 2024
…` flags.

See prometheus/proposals#41 for rationale.

Prometheus job got updated with prometheus/prometheus#15682

Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
Copy link
Member

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

Approved with nit comment.

proposals/2024-12-04_prombench-custom-benchmarks.md Outdated Show resolved Hide resolved
Co-authored-by: George Krajcsovits <krajorama@users.noreply.github.com>
Signed-off-by: Bartlomiej Plotka <bwplotka@gmail.com>
Copy link
Member

@ArthurSens ArthurSens left a comment

Choose a reason for hiding this comment

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

LGTM, we can proceed with the alternative in another proposal if we ever feel like it :)

bwplotka added a commit to prometheus/test-infra that referenced this pull request Dec 19, 2024
…` flags. (#812)

* comment-monitor: Added flag support; added built-in help command.

Implements first phase of prometheus/proposals#41

Signed-off-by: bwplotka <bwplotka@gmail.com>

* prombench: Added support for `--bench.version` and `--bench.directory` flags.

See prometheus/proposals#41 for rationale.

Prometheus job got updated with prometheus/prometheus#15682

Signed-off-by: bwplotka <bwplotka@gmail.com>

* extended comment to include version and directory used.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* addressed comments.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* Makefile fix.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* rm unnecessary print.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* Improved help.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* Fixed help.

Signed-off-by: bwplotka <bwplotka@gmail.com>

* Added missed test file.

Signed-off-by: bwplotka <bwplotka@gmail.com>

---------

Signed-off-by: bwplotka <bwplotka@gmail.com>
Signed-off-by: bwplotka <bwplotka@gmail.com>
@bwplotka bwplotka merged commit a06b6b6 into main Dec 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants