Skip to content

Cherry pick some dropped Gerrrr commits - #127

Closed
StephanDollberg wants to merge 2 commits into
apache:masterfrom
StephanDollberg:stephan/gerrrr-dropped-commits
Closed

StephanDollberg wants to merge 2 commits into
apache:masterfrom
StephanDollberg:stephan/gerrrr-dropped-commits

Conversation

@StephanDollberg

Copy link
Copy Markdown

It looks like various commits got dropped when importing the project from Gerrrr/hunter:

This commit cherry picks:

Sometimes users want to know about *any* performance change between a PR
and a baseline branch, i.e. not just whether there's a regression but
also if there's a performance improvement.

(cherry picked from commit 116eb5a)
It's useful to get the output of "hunter regressions" in JSON format for
post-processing. The default behaviour if the user doesn't pass
--output should be the same as before this change.

(cherry picked from commit 3400a3c)
@henrikingo

Copy link
Copy Markdown
Contributor

We are removing the entire regressions command.

If you were using it, you are welcome to explain what it is good for. (genuinely)

@StephanDollberg

Copy link
Copy Markdown
Author

We use it for feature branch/PR regression testing. Pretty much exactly as described here: https://github.com/Gerrrr/hunter?tab=readme-ov-file#validating-performance-of-a-feature-branch

@Gerrrr

Gerrrr commented Feb 17, 2026

Copy link
Copy Markdown
Contributor

Hey Stephan! It is awesome to learn that there are more Otava users in the wild.

As Henrik mentioned, we've been discussing dropping regressions command in this PR #124, with the full discussion being in #118.

The TL;DR of those discussions is:

  1. regressions command has no real advantage over analyze.
  2. As we drop regressions, we are updating docs on how to test feature branches with analyze in https://github.com/apache/otava/pull/124/changes#diff-a98c0d6ce01e6aed1bc70cad83b2588384261bc35308a238102d81a2a02a2e44.

Can you please take a look if there is anything that would be missing for you to switch to analyze? Meanwhile, I will hold off with merging my PR to hear your thoughts.

Best,
Alex

@StephanDollberg

Copy link
Copy Markdown
Author

It is awesome to learn that there are more Otava users in the wild

Yeah we definitely use it a lot 😁 . Both for change point detection via analyze and also just for PR regression/improvement via regressions.

regressions command has no real advantage over analyze.

Ok sounds great. I am happy to migrate if there is feature parity.

Can you please take a look if there is anything that would be missing for you to switch to analyze?

I can have a more detailed look tomorrow. From a quick look at the docs I am wondering whether there is similar functionality to:

  • --since-commit: We want to compare to the correct merge base
  • --last 1: For PRs we really only care about the last run
  • --ignore-direction: This is really useful for us because we don't really care about the direction just whether there has been a positive or negative change

Thanks for the quick replies. I'll take a deeper look tomorrow.

@henrikingo

Copy link
Copy Markdown
Contributor

Partial answer, as I never really used regressions myself, but otoh I'm probably the one with strongest/most confident opinions on these topics, so you can always blame me for bad decisions...

--ignore-direction This is essentially always the case now (and always was in analyze). If you wanted to get only negative change points, you'd have to filter out positive changes by checking the direction field. Like you, I strongly believe that a lot of the positive changes turn out to be bugs upon closer analysis, so we should not even allow the option of ignoring them.

--last 1 If analyze doesn't have this, it could be added. In nyrkio we use otava in PRs, and it works great. We simply run it normally, then check whether there's a change point on the last comit or not, and ignore everything else.

--since-commit This can be broken down into two use cases:

  • You want to compare two different versions/builds. A feature branch vs main branch, or the main branch against a stable branch (previous release). My opinion on this one is that it is not completely wrong approach, but this is not what Otava is designed to do. It is not change point detection, it is more like A/B testing or something.
  • Alternative syntax to --last N, IMO this can be supported if there's demand. It is just a convenience method to filter out some of the results, jst like last 1 or min threshold. But note that comparing against the merge-base is actually not necessarily what you want to do: In a feature branch, it is not necessarily wrong or a problem to find change points. The question is just have you fixed or otherwise addressed all of them. So using --since or --last with a requirement/expectation that no change points should be found, isn't correct in general case.'

@StephanDollberg

Copy link
Copy Markdown
Author

--ignore-direction This is essentially always the case now

Sounds good

that a lot of the positive changes turn out to be bugs upon closer analysis

100%

whether there's a change point on the last comit or not, and ignore everything else.

Right, that's a fine workaround though having it builtin would be convenient.

You want to compare two different versions/builds. A feature branch vs main branch, or the main branch against a stable branch (previous release). My opinion on this one is that it is not completely wrong approach, but this is not what Otava is designed to do. It is not change point detection, it is more like A/B testing or something.

Right, we definitley use it for A/B testing. For us the advantage of that is that like this (A: dev/master/main/mergebase, B: PR) we don't have re-run A at all. We just use our existing master data and use otava/change-point-detection to find the correct data to compare to (which the is the point the PR/branch forked from master. Otherwise you might get differences based on changes that are just further ahead on master)

We also don't really care about old changepoints for the PR as the latest version is all that matters. But sure we could do some filtering on our side for only the last dataset on the branch/PR.

@henrikingo

Copy link
Copy Markdown
Contributor

Otherwise you might get differences based on changes that are just further ahead on master

Ah that's right. I forgot about this.

Is that what --since-commit does/did? I thought it was essentially a synonym for --last???

Can you explain a bit more, how do you provide the data to Otava when you are dealing with a pull request? (I'm not sure this works the same with all the databases...)

I just realized in some cases a PR would not be anything special: It is just a history of git commits (child->parent) and it starts (or ends, rather) with the tip of the feature branch. On the other hand if you think of the PR branch as just one or a few commits, then appending them to the end of the main branch is indeed often wrong.

It seems I use the commit time to determine the right position of the pr commit in the main branch history. I remember spending some time convincing myself that this is equivalent to looking at merge-base or the chain of parent refrerenses.

@StephanDollberg

Copy link
Copy Markdown
Author

Is that what --since-commit does/did? I thought it was essentially a synonym for --last???

With --since-commit it tries to find the stable range around that commit (range in between the closest changepoint before and after that commit). This is important because of the fact mentioned above with possible later changes. --last I'd expect to behave differently?

Can you explain a bit more, how do you provide the data to Otava when you are dealing with a pull request? (I'm not sure this works the same with all the databases...)

Sure, so we use the json provider (not sure whether that's the right word). Then we provide the following data:

  • All the data points from master. These get created on merge to master so we have data for all commits on master
  • All the data points for runs from the PR/branch. This will only ever be one latest data point as we don't run the benchmarks for each individual commit on a PR. It could be multiple data points though in case users trigger the A/B comparison multiple times (after pushing changes). For the latter bit is what we use --last 1. As mentioned above we could work around this by only providing the last dataset to hunter on our side. Just haven't done this for now as hunter already offered --last 1 so it was easy to do that way.

It seems I use the commit time to determine the right position of the pr commit in the main branch history. I remember spending some time convincing myself that this is equivalent to looking at merge-base or the chain of parent refrerenses.

Yeah hunter/otava isn't really git aware so it's all just time based. This can cause confusion (it did for us initially) but works fine once you get the timestamps right that you pass to hunter (i.e.: use commit timestamps from the merge commits or similar).

@henrikingo

Copy link
Copy Markdown
Contributor

Ok thanks. So this is in the end exactly what I do too, you just thought about it and described it differently. (The Json data source is contributed by me, even :-)

...it seems I didn't contribute any documentation though. Filed #128

The CLI options are evolving for all importers, hopefully the json importer is included by whoever works on those.

Anyway, in summary, what I've found works well is that you take your test result history until the merge-base, then add the other branch as a single data point to the end of that time series. (There seems to be both --since and --until options. Personally I deal with this outside of Otava.) And for the purposes of a PR we are only interested in whether this last data point is a change point. (--last 1)

So as we evolve the user interface options in Otava, we can keep this in mind. On the mailing list I just proposed a separation of core functionality of the algorithm versus more complex management of exactly things like which commits to include in the first place, or the nature of a PR.

Note by the way there is nowadays also an incremental mode, where if you have stored the state of a previous computation, you can cheaply calculate the algorithm only for the tail of the series, because new points are only appended to the end: https://github.com/apache/otava/blob/master/otava/series.py#L325-L407

...again, this is not exposed in the CLI, and of course using it requires you have available the state from the previous computation of change points.

@StephanDollberg

Copy link
Copy Markdown
Author

Thanks, I have now reimplemented our regression usage with analyze plus some manual filtering for --since-commit and --last.

It seems to be working fine. Closing this PR.

@Gerrrr Gerrrr mentioned this pull request Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants