Use correct commit hashes when uploading metric reports.

[yifanyang]

This pull request addresses these two issues:

• Currently the metrics report is labeled with the commit hash from environment variable PULL_PULL_SHA. However, that is the hash of the PR head commit, but not the merge commit produced by merging the PR head commit into the master branch, which all the CI tests actually run against.
• Currently the base_commit is using the commit hash from PULL_BASE_SHA, which is not necessarily the most recent master commit at the time of test on CI. That misleads the metrics service to compare to some older commit in master branch, but not the immediate master commit used to produce the merge commit.

[vkryachko]

@yifanyang I can see how the base sha being stale could cause problems, but I don't understand why we care about the merge commit sha, iiuc the merge commit is only meaningful during the CI run, what is the purpose of recording this sha in metrics?

[yifanyang]

what is the purpose of recording this sha in metrics?

1. This sha is also displayed in the report. E.g. in this comment, the numbers in the "Head (704f568)" column are not really from commit 704f568, but the CI merge commit with 704f568 being one of its parent.
2. Also, if a future PR happens to use 704f568 as its comparison base (this will only happen in the case where the PR is merging to some feature branch rather than master, which is not supported now), the backend would incorrectly assume the data it has is from 704f568, while it is actually from another commit. Feature branch PRs are not supported now as we only run post-submit tests for master branch, therefore in a feature branch PR, the data for base commit is always missing. But it is still a potential problem.

I have configured JS sdk to use the merge commit and enabled feature branch support. One convenience from JS sdk is that their merge commits are produced by github, therefore if one hover over the "Head (f567375)" in the report, it actually shows that the commit is merging A into B, where A is the last push commit in the PR and B is the base commit displayed in the same report. Our merge commit are produced by prow, so we will loose this feature, which can be a drawback.

With all that said, do you think I should pursue this approach?

[vkryachko]

Our merge commit are produced by prow, so we will loose this feature, which can be a drawback.

This is actually my main concern, since this sha is unknown to github it might be confusing to have it in the report as it's impossible to see what this sha refers to.
I don't feel strongly so feel free to merge as is, but I wanted to call it out.

[yifanyang]

I think it is a valid concern. Here is what I'm going to do:

• The metrics service will accept another optional query param note
• Uploader here will pass the commit info (git show HEAD --format=full -s) via this field
• A report will render another section note with the content in this field

The coverage report already contains a note section describing how to ran coverage task locally. Let me make this feature more generalized.