You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@otava.apache.org by "Gerrrr (via GitHub)" <gi...@apache.org> on 2025/12/22 22:56:42 UTC
[I] Fix branch interpolation logic in regressions command [otava]
Gerrrr opened a new issue, #118:
URL: https://github.com/apache/otava/issues/118
The `otava regressions` command supports an optional `--branch` argument, which specifies the feature branch to compare against the base branch.
## Problem
The current implementation has several limitations:
1. The feature-branch logic is implemented at the importer level, which forces every importer to either implement it or explicitly raise `NotImplementedError` (as is currently the case).
2. The Graphite importer relies on custom interpolation logic for the `$BRANCH` environment variable.
3. There is no way to configure the base branch name.
These limitations lead to several undesirable consequences:
1. To configure a custom base branch, we we must manually expand the `$BRANCH` environment variable (via `expandvars`). PostgreSQL example and e2e test illustrate this problem.
2. Feature-branch testing is not supported by importers other than Graphite.
3. Importer behavior is inconsistent and difficult to reason about.
## Proposed Solution
### CLI API Changes
Replace the existing `--branch` argument with two explicit arguments:
- `--base-branch`
Specifies the base branch to compare against.
Default: `main`
- `--feature-branch`
Specifies the feature branch to compare.
No default.
**Usage semantics:**
- If only `--base-branch` is provided, it overrides the default base branch and tests for regressions within that branch.
- If both `--base-branch` and `--feature-branch` are provided, Otava tests the feature branch for regressions relative to the base branch.
- If only `--feature-branch` is provided, Otava compares the feature branch against the default base branch (`main`).
### Configuration Interpretation Changes
1. **`regressions` command**
Otava expands the `$BRANCH` variable using both the base and feature branch values.
2. **`analysis` command**
Otava expands `$BRANCH` using the value provided via `--branch`, following the same mechanism as other `ConfigArgParse`-controlled options.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "Gerrrr (via GitHub)" <gi...@apache.org>.
Gerrrr commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3688169108
Re branch names, I am slightly against "head branch" because I find it a bit confusing and, as a user, would have look up what each of those do. I don't think there is standard terminology, as Gitlab calls those "target branches" and Bitbucket calls them "destination branches".
For base branch, I equally prefer base branch and main branch.
> Regarding the regressions command, what does the basic version of that do?
Unlike `analyze`, `regressions` always fetches 30 days worth of data on the base branch and then finds regressions within the specified selector - https://github.com/apache/otava/blob/3b3d59a787ea03749e972c3509241114b861c99e/otava/main.py#L264-L270.
It makes sense, but I wonder why don't we do this in `analyze` as well as it seems to generally make sense. If we'd do that, it would only be valuable to run `regressions` to analyze feature branches... however we could just move feature branch comparison into `analyze` and remove `regressions` command.
> Then, if I understood what is the intended use case we are talking about, my next question would be, why do we need variable interpolation for this use case?
Consider a use case, a benchmark that you run and want to analyze across multiple versions. One way to approach that is to have multiple identical tests that only differ in the branch name. Another is to make branch name a variable.
Here is a query that is very similar to one of the use cases I care about:
```
tests:
aggregate_mem:
inherit: [ common ] # avoids repeating metrics definitions and postgres-related config
query: |
SELECT e.commit,
e.commit_ts,
r.process_cumulative_rate_mean,
r.process_cumulative_rate_stderr,
r.process_cumulative_rate_diff,
r.experiment_id,
r.config_id
FROM results r
INNER JOIN configs c ON r.config_id = c.id
INNER JOIN experiments e ON r.experiment_id = e.id
WHERE e.exclude_from_analysis = false AND
e.branch = '${BRANCH}' AND
e.username = 'ci' AND
c.store = 'MEM' AND
c.cache = true AND
c.benchmark = 'aggregate' AND
c.instance_type = 'ec2i3.large'
ORDER BY e.commit_ts ASC;
```
Then, it is called like that:
```
export BRANCH=... # can be trunk, 3.4, 3.5, etc
otava analyze all --update-postgres --output regressions_only --last 10
```
`ConfigArgParse` does not expand `$BRANCH`, so I had to bring `expandvars` back. At the same time, I agree with your take from the mailing list that it would be nice to get rid of `expandvars` completely. Why can't we do it right away and ConfigArgParse'ify `--branch`? The problem is with feature-branch comparison that needs to query the time series and expand it twice with 2 different values - one for the base branch and one for the feature branch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3697036905
So, let's then focus on the most interesting question of yaml and/or cli api for options and test definitions. Thanks for sharing an example. I actually had not realized you can put entire sql statements in the yaml files. This must have been added when you added pgsql support? I remember the prometheus and of course csv cases as simpler than this, but I could remember wrong of course.
But okay, it seems we have created a system where users can essentially analyze an arbitrary dataset, connect to many differrent data sources., and do that differently for each test even.. It is starting to feel like we are building Tableu, not just a small utility to help with CI... But I'm ok with that, should it happen.
So, realistically we'll have to support a subset of the more complex configuration as yaml only? IMO it would be good if this can be split into a completely separate file, so that the config that is managed with ConfigArgParse is clearly separate and also "just works" without needing to hack ConfigArgParse.
For the inherit functionality I still feel this is re-inventing a standard yaml feature (anchors) but I don't feel channging thatis the most urgent thing in the world.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3687148263
Regarding the regressions command, what does the basic version of that do? Is it just analyze, but with improvements / positive changes removed? I've tried to read the docs also when we did the name change, and this feature feels somehow overloaded to me.
The task of comparing the tail of two branches is IMO meaningful. But I don't understand what this has to do with regressions.
Then, if I understood what is the intended use case we are talking about, my next question would be, why do we need variable interpolation for this use case? Why can't it just compare the two branches that are given on the command line? (Or config file, env vars... as supported by ConfigArgParse.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3696068948
Ok, so based on your explanation,
- regressions sounds like something Piotr and Shaunak have invented and does something very specific to Datastax internal processes. Similar to the functionality of sending a slack notification once per week and including only 14 days of history in the notification. (As opposed to sending a notification when a regression was found the first time, remembering that you've done so, and possibly also tracking whether the regression was fixed or not.)
- While it is understandable that from an engineering point of view you will not be interested in change points that happened 3 years back... I feel it is againnst the spirit of change point detection to only focus on the most recent tail of the timeseries. Even if you are only interested in recent regressions, the historical data is used by the algorithm to build a model of the noise and distribution of the data. Arbitrarily discarding most of the historical data is a little bit wrong.
- Although the act of comparing a feature branch against a stable branch is IMO a valid problem, it is actually a different problem and potentially different solution. A change point detection solution would be to simply analyze the history of the feature branch as a single, independent history. Or rather, not completely independent, as a branch by definition shares a common history with all other "branches" going back to the "trunk". If there are no change points in the history of the branch, then there are no regressions. If the main/target branch got faster for unrelated reasons, this should still be uninteresting to the developer of the feature branch.
- I would be willing to propose we remove the regressions command entirely, including the --branch option.
Users should instead:
- simply run otava over the history of the feature branch. There is no need to compare it to any other branch.; OR
- GitHub-style workflow: Create a pull request, which will run benchmarks where the feature branch is rebased and merged onto the head of the main/stable branch. It turns out this reduces to a special case of the previous point: The temporary branch created by a rebase+merge, is actually just a feature branch that shares a common history with the main.
As a special concession, we can implement some kind of option to say that we are only interested in change points that happen in the feature branch part, But that is essentially --since already.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3696106775
Re: naming, let's then not attach ourselves to github's semantics. I like "target branch" for the main branch. I would avoid "head, main and even base branch" for the feature branch. Maybe "source branch", "candidate branch"or "incoming branch"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3687081050
Since I spent a lot of time with the github api this year, just want to document here that in the pull request api, the two branches are called base branch and head branch. (main/master branch is the base)
IMO something like main and feature branch is equally, if not more intuitive. But there might be value in following industry standard.
Also, in the context of pull requests, I find that "target" is also a good name for the base branch, since the intention is for the target branch to receive new commits. However, that might not always be the context, so maybe indeed base branch and head or feature branch is the best.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "Gerrrr (via GitHub)" <gi...@apache.org>.
Gerrrr commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-3735760450
Hey @henrikingo ! Sorry for the long reply. I am finally back from Christmas/New Year holidays and caught up with work.
> I actually had not realized you can put entire sql statements in the yaml files. This must have been added when you added pgsql support?
Yes, this is helpful in cases, where the same benchmark runs with different configurations, multiple environments environments (e.g, with in-memory vs on-disk storage, on different instances, etc), and against multiple branches (e.g., all supported Kafka versions).
> I would be willing to propose we remove the regressions command entirely, including the --branch option.
+1 to removing `regressions` command.
To clarify, what do you think about ConfigArgParsed `--branch BRANCH` in the `analyze` command? We already support `analyze --branch`, but it can be configured only with a CLI argument. While I 100% agree now that we do not really need any special functionality to support feature branch analysis (as we do not want to do any extra comparisons with the base branch), it is almost always helpful in my cases to pass the branch name to a test, because the same experiment usually runs against many branches.
> IMO it would be good if this can be split into a completely separate file, so that the config that is managed with ConfigArgParse is clearly separate and also "just works" without needing to hack ConfigArgParse.
Can you please elaborate on this? What would you put into the main config vs new separate files? At this point, it does not look like we need to hack ConfigArgParse any longer; all we need to do to satisfy the initial motivation for this ticket is to:
1. Remove `regressions` command.
2. ConfigArgParsify `--branch` in `analyze` command.
> I still feel this is re-inventing a standard yaml feature (anchors)
Hah, I did not know YAML achors exist. +1 replacing templates with acnhors. Let's create a ticket .
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Fix branch interpolation logic in regressions command [otava]
Posted by "henrikingo (via GitHub)" <gi...@apache.org>.
henrikingo commented on issue #118:
URL: https://github.com/apache/otava/issues/118#issuecomment-4245476304
@gerr Is there any reason to keep this ticket open?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@otava.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org