Fix branch interpolation logic in regressions command

[Gerrrr]

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.

[henrikingo]

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.

[henrikingo]

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.)

[Gerrrr]

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 -

otava/otava/main.py

Lines 264 to 270 in 3b3d59a

	# Even if user is interested only in performance difference since some point X,
	# we really need to fetch some earlier points than X.
	# Otherwise, if performance went down very early after X, e.g. at X + 1, we'd have
	# insufficient number of data points to compute the baseline performance.
	# Instead of using `since-` selector, we're fetching everything from the
	# beginning and then we find the baseline performance around the time pointed by
	# the original selector.

.

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.

[henrikingo]

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.

[henrikingo]

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"

[henrikingo]

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.