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

Prefer committer over author date for perf results timestamp #48673

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

WunderBart
Copy link
Member

@WunderBart WunderBart commented Mar 2, 2023

What?

Related: #48243 (comment)

Prefer the commiter date over the author date for logging performance tests results.

Why?

There's a mismatch between COMMITTED_AT and commit_details.data.author.data whereby the author can choose a time for the commit, but the actual time of the commit is different. From the docs:

In Git, the author date is when someone first creates a commit with git commit. The commit date is identical to the author date unless someone changes the commit date by using git commit --amend, a force push, a rebase, or other Git commands.

How?

Get the committer timestamp directly via git instead of fetching via the script action as suggested by @dmsnell here.

@WunderBart WunderBart added [Type] Performance Related to performance efforts GitHub Actions Pull requests that update GitHub Actions code labels Mar 2, 2023
@WunderBart WunderBart self-assigned this Mar 2, 2023
Base automatically changed from add/perf-tests-artifacts-upload to trunk March 2, 2023 09:59
In Git, the author date is when someone first creates a commit with git commit. The commit date is identical to the author date unless someone changes the commit date by using git commit --amend, a force push, a rebase, or other Git commands.
@WunderBart WunderBart force-pushed the fix/perf-results-committed-at branch from 3d64a71 to 3fa52ea Compare March 2, 2023 10:18
Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

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

I'm not sure how to test this apart from merging and watching, so if you will do that then it seems good.

It's good to get rid of the extra step in the job. Also I'm guessing we have out-of-order commits in codehealth because of having been reporting the author date. I doubt that's significant for the purpose of tracking these things over time, but I feel like I've seen some commits where the author date was weeks behind the commit.

@WunderBart WunderBart merged commit 0771b0a into trunk Mar 7, 2023
@WunderBart WunderBart deleted the fix/perf-results-committed-at branch March 7, 2023 14:37
@github-actions github-actions bot added this to the Gutenberg 15.4 milestone Mar 7, 2023
@ellatrix
Copy link
Member

ellatrix commented Jun 23, 2023

The ordering by date should be fixed by Perf logging: change date to ISO 8601.

@dmsnell
Copy link
Member

dmsnell commented Jun 24, 2023

@ellatrix what are you referring to? did you mean to link to this same PR in your comment?

@ellatrix
Copy link
Member

Huh, no idea why that linked to the this PR. I meant: #51833

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GitHub Actions Pull requests that update GitHub Actions code [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants