From 09914bcab7d1d5570c20a52d1861957145da8a5a Mon Sep 17 00:00:00 2001 From: Ram Peri Date: Fri, 7 Aug 2026 21:54:45 +0000 Subject: [PATCH] Add detailed upload context to Trace2 telemetry This patch intercepts the output during a successful upload execution to capture the generated CL URLs. It logs a dynamically constructed "repo.uploadstate" data event to the active trace2 log containing: - Uploaded CL URLs - Target remote name - Source branch - Modified files Test: 1. ./run_tests 2. pytest tests/test_project.py 3. Manual verification: - Created a dummy branch `test_upload_branch` in a project with local file changes. - Invoked repo upload passing an explicit trace output directory: `repo --git-trace2-event-log=/tmp/trace2out upload --dry-run --no-verify art` - Verified the injected payload successfully appeared on disk within the generated log: ```json {"event":"data",..."key":"repo.uploadstate/cls","value":""} {"event":"data",..."key":"repo.uploadstate/remote","value":"goog"} {"event":"data",..."key":"repo.uploadstate/branch","value":"test_upload_branch"} {"event":"data",..."key":"repo.uploadstate/files","value":"dummy_file.txt"} ``` (cls is correctly blank on --dry-run but populates on real HTTP pushes) Bug: 543953499 Change-Id: I9c402a32d01d156d42cf24eaa2e60e22710b5e6f Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/616581 Reviewed-by: Gavin Mak Tested-by: Ram Peri Commit-Queue: Ram Peri --- project.py | 48 +++++++- subcmds/upload.py | 1 + tests/test_project.py | 259 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 305 insertions(+), 3 deletions(-) diff --git a/project.py b/project.py index 2b0019070..1c3d8bf21 100644 --- a/project.py +++ b/project.py @@ -239,6 +239,12 @@ class ReviewableBranch: "--pretty=format:%cd", "-n", "1", R_HEADS + self.name, "--" ) + @property + def modified_files(self) -> List[str]: + return self.project.bare_git.diff( + "--name-only", f"{self.base}...{R_HEADS}{self.name}" + ).splitlines() + @property def base_exists(self): """Whether the branch we're tracking exists. @@ -271,7 +277,8 @@ class ReviewableBranch: validate_certs=True, push_options=None, patchset_description=None, - ): + git_event_log: Optional[EventLog] = None, + ) -> None: self.project.UploadForReview( branch=self.name, people=people, @@ -287,6 +294,7 @@ class ReviewableBranch: validate_certs=validate_certs, push_options=push_options, patchset_description=patchset_description, + git_event_log=git_event_log, ) def GetPublishedRefs(self): @@ -1193,7 +1201,8 @@ class Project: validate_certs=True, push_options=None, patchset_description=None, - ): + git_event_log: Optional[EventLog] = None, + ) -> None: """Uploads the named branch for code review.""" if branch is None: branch = self.CurrentBranch @@ -1282,14 +1291,47 @@ class Project: ref_spec = ref_spec + "%" + ",".join(opts) cmd.append(ref_spec) - GitCommand(self, cmd, bare=True, verify_command=True).Wait() + push_cmd = GitCommand( + self, + cmd, + bare=True, + verify_command=True, + ) + push_cmd.Wait() + cls_urls = self._FindGerritUrls(push_cmd.stderr) + + try: + rb = ReviewableBranch(self, branch, branch.LocalMerge) + modified_files_list = rb.modified_files + if git_event_log: + git_event_log.LogDataConfigEvents( + { + "cls": ",".join(cls_urls), + "remote": branch.remote.name, + "branch": branch.name, + "files": ",".join(modified_files_list), + }, + "repo.uploadstate", + ) + except Exception as e: + logger.error("Tracing failed: %s", str(e)) if not dryrun: msg = f"posted to {branch.remote.review} for {dest_branch}" self.bare_git.UpdateRef( R_PUB + branch.name, R_HEADS + branch.name, message=msg ) + @staticmethod + def _FindGerritUrls(stderr: Optional[str]) -> List[str]: + """Extracts Gerrit review URLs from git push output.""" + if not stderr: + return [] + return [ + match.group(1) + for match in re.finditer(r"(https?://[^/]+/c/.+?/\+/\d+)", stderr) + ] + @staticmethod def _encode_patchset_description(original): """Applies percent-encoding for strings sent as patchset description. diff --git a/subcmds/upload.py b/subcmds/upload.py index 49d8e2e5f..56a2066e7 100644 --- a/subcmds/upload.py +++ b/subcmds/upload.py @@ -649,6 +649,7 @@ Gerrit Code Review: https://www.gerritcodereview.com/ validate_certs=opt.validate_certs, push_options=push_options, patchset_description=opt.patchset_description, + git_event_log=self.git_event_log, ) branch.uploaded = True diff --git a/tests/test_project.py b/tests/test_project.py index 8c13c4f0e..7d77ed8b6 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -29,6 +29,7 @@ import utils_for_test import error import git_config +import git_trace2_event_log import manifest_xml import platform_utils import project @@ -82,6 +83,7 @@ class ReviewableBranchTests(unittest.TestCase): short, long = next(iter(d.items())) self.assertTrue(long.startswith(short)) self.assertTrue(rb.base_exists) + self.assertEqual(["readme"], rb.modified_files) # Hard to assert anything useful about this. self.assertTrue(rb.date) @@ -95,6 +97,20 @@ class ReviewableBranchTests(unittest.TestCase): # Hard to assert anything useful about this. self.assertTrue(rb.date) + def test_upload_for_review_forwards_git_event_log(self) -> None: + """Check UploadForReview passes git_event_log to project.""" + proj = mock.MagicMock(spec=project.Project) + branch = mock.MagicMock() + branch.name = "work" + rb = project.ReviewableBranch(proj, branch, "main") + mock_event_log = mock.MagicMock() + + rb.UploadForReview(people=([], []), git_event_log=mock_event_log) + + proj.UploadForReview.assert_called_once() + _, kwargs = proj.UploadForReview.call_args + self.assertEqual(kwargs.get("git_event_log"), mock_event_log) + class ProjectTests(unittest.TestCase): """Check Project behavior.""" @@ -105,6 +121,249 @@ class ProjectTests(unittest.TestCase): "abcd00%21%21_%2b", ) + def test_find_gerrit_urls(self) -> None: + """Check _FindGerritUrls extracts review URLs from stderr.""" + # Single CL URL from standard push output. + stderr = ( + "remote:\n" + "remote: Processing changes: new: 1, refs: 1, done\n" + "remote:\n" + "remote: SUCCESS\n" + "remote:\n" + "remote: https://gerrit.example.com/c/git-repo/+/616581" + " Add telemetry [NEW]\n" + "remote:\n" + "To sso://gerrit/git-repo\n" + " * [new reference] HEAD -> refs/for/main\n" + ) + self.assertEqual( + project.Project._FindGerritUrls(stderr), + ["https://gerrit.example.com/c/git-repo/+/616581"], + ) + + # Project names containing slashes and nested paths. + url1 = "https://example.com/c/platform/base/+/12345" + url2 = "https://example.com/c/vendor/device/raviole/prebuilts/+/987654" + stderr_nested = f"remote: {url1}\nremote: {url2} [NEW]\n" + self.assertEqual( + project.Project._FindGerritUrls(stderr_nested), + [url1, url2], + ) + + # Multiple URLs on same line or custom ports / http schemas. + stderr_custom = ( + "remote: https://review.corp:8443/c/platform/manifest/+/4321\n" + "remote: http://localhost:8080/c/test-project/+/555\n" + ) + self.assertEqual( + project.Project._FindGerritUrls(stderr_custom), + [ + "https://review.corp:8443/c/platform/manifest/+/4321", + "http://localhost:8080/c/test-project/+/555", + ], + ) + + # Non-matching outputs. + self.assertEqual(project.Project._FindGerritUrls(None), []) + self.assertEqual(project.Project._FindGerritUrls(""), []) + self.assertEqual( + project.Project._FindGerritUrls("Everything up-to-date\n"), [] + ) + self.assertEqual( + project.Project._FindGerritUrls( + "https://example.com/not/a/gerrit/url" + ), + [], + ) + + def _create_project_for_upload_test( + self, + ) -> Tuple[mock.MagicMock, mock.MagicMock]: + proj = mock.MagicMock(spec=project.Project) + proj.name = "test-project" + proj.UserEmail = "test@example.com" + proj.dest_branch = "refs/heads/main" + proj.bare_git = mock.MagicMock() + proj._FindGerritUrls = project.Project._FindGerritUrls + + mock_branch = mock.MagicMock() + mock_branch.name = "test-branch" + mock_branch.LocalMerge = "refs/heads/main" + mock_branch.merge = "refs/heads/main" + mock_branch.remote.review = "http://review.example.com" + mock_branch.remote.name = "origin" + mock_branch.remote.projectname = "test-project" + mock_branch.remote.ReviewUrl.return_value = ( + "https://review.example.com/test-project" + ) + + proj.GetBranch.return_value = mock_branch + return proj, mock_branch + + def test_upload_for_review_event_emission(self) -> None: + """Check UploadForReview emits repo.uploadstate trace2 data events.""" + proj, _ = self._create_project_for_upload_test() + + with mock.patch("project.GitCommand") as mock_git_cmd, mock.patch( + "project.ReviewableBranch" + ) as mock_rb_cls: + mock_cmd = mock.MagicMock() + mock_cmd.Wait.return_value = 0 + mock_cmd.stderr = ( + "remote: https://example.com/c/test/+/123 [NEW]\n" + "remote: https://example.com/c/test/+/124 [NEW]\n" + ) + mock_git_cmd.return_value = mock_cmd + + mock_rb = mock.MagicMock() + mock_rb.modified_files = ["file1.txt", "file2.txt"] + mock_rb_cls.return_value = mock_rb + + mock_event_log = mock.MagicMock() + project.Project.UploadForReview( + proj, + branch="test-branch", + dryrun=True, + git_event_log=mock_event_log, + ) + + mock_event_log.LogDataConfigEvents.assert_called_once_with( + { + "cls": ( + "https://example.com/c/test/+/123," + "https://example.com/c/test/+/124" + ), + "remote": "origin", + "branch": "test-branch", + "files": "file1.txt,file2.txt", + }, + "repo.uploadstate", + ) + + def test_upload_for_review_event_emission_no_cls(self) -> None: + """Check event emission when stderr contains no review URLs.""" + proj, _ = self._create_project_for_upload_test() + + with mock.patch("project.GitCommand") as mock_git_cmd, mock.patch( + "project.ReviewableBranch" + ) as mock_rb_cls: + mock_cmd = mock.MagicMock() + mock_cmd.Wait.return_value = 0 + mock_cmd.stderr = "Everything up-to-date\n" + mock_git_cmd.return_value = mock_cmd + + mock_rb = mock.MagicMock() + mock_rb.modified_files = ["dummy.txt"] + mock_rb_cls.return_value = mock_rb + + mock_event_log = mock.MagicMock() + project.Project.UploadForReview( + proj, + branch="test-branch", + dryrun=True, + git_event_log=mock_event_log, + ) + + mock_event_log.LogDataConfigEvents.assert_called_once_with( + { + "cls": "", + "remote": "origin", + "branch": "test-branch", + "files": "dummy.txt", + }, + "repo.uploadstate", + ) + + def test_upload_for_review_no_event_log(self) -> None: + """Check UploadForReview succeeds when git_event_log is None.""" + proj, _ = self._create_project_for_upload_test() + + with mock.patch("project.GitCommand") as mock_git_cmd, mock.patch( + "project.ReviewableBranch" + ) as mock_rb_cls: + mock_cmd = mock.MagicMock() + mock_cmd.Wait.return_value = 0 + mock_cmd.stderr = "remote: https://example.com/c/test/+/1\n" + mock_git_cmd.return_value = mock_cmd + + mock_rb = mock.MagicMock() + mock_rb.modified_files = ["file.txt"] + mock_rb_cls.return_value = mock_rb + + project.Project.UploadForReview( + proj, + branch="test-branch", + dryrun=True, + git_event_log=None, + ) + + def test_upload_for_review_tracing_exception_handled(self) -> None: + """Check exceptions during tracing are caught and do not fail upload.""" + proj, _ = self._create_project_for_upload_test() + + with mock.patch("project.GitCommand") as mock_git_cmd, mock.patch( + "project.ReviewableBranch" + ) as mock_rb_cls, mock.patch("project.logger") as mock_logger: + mock_cmd = mock.MagicMock() + mock_cmd.Wait.return_value = 0 + mock_cmd.stderr = "remote: https://example.com/c/test/+/1\n" + mock_git_cmd.return_value = mock_cmd + + mock_rb_cls.side_effect = Exception("failed to inspect branch") + + mock_event_log = mock.MagicMock() + project.Project.UploadForReview( + proj, + branch="test-branch", + dryrun=True, + git_event_log=mock_event_log, + ) + + mock_logger.error.assert_called_once() + mock_event_log.LogDataConfigEvents.assert_not_called() + + def test_upload_for_review_real_event_log(self) -> None: + """Check integration with real git_trace2_event_log.EventLog.""" + proj, _ = self._create_project_for_upload_test() + + with mock.patch("project.GitCommand") as mock_git_cmd, mock.patch( + "project.ReviewableBranch" + ) as mock_rb_cls: + mock_cmd = mock.MagicMock() + mock_cmd.Wait.return_value = 0 + mock_cmd.stderr = ( + "remote: https://example.com/c/test/+/456 [NEW]\n" + ) + mock_git_cmd.return_value = mock_cmd + + mock_rb = mock.MagicMock() + mock_rb.modified_files = ["file1.py", "file2.py"] + mock_rb_cls.return_value = mock_rb + + event_log = git_trace2_event_log.EventLog(env={}) + project.Project.UploadForReview( + proj, + branch="test-branch", + dryrun=True, + git_event_log=event_log, + ) + + data_events = [ + e for e in event_log._log if e.get("event") == "data" + ] + data_map = {e["key"]: e["value"] for e in data_events} + self.assertEqual( + data_map.get("repo.uploadstate/cls"), + "https://example.com/c/test/+/456", + ) + self.assertEqual(data_map.get("repo.uploadstate/remote"), "origin") + self.assertEqual( + data_map.get("repo.uploadstate/branch"), "test-branch" + ) + self.assertEqual( + data_map.get("repo.uploadstate/files"), "file1.py,file2.py" + ) + def test_get_head_revision_id(self): """Check GetHeadRevisionId behavior.""" with utils_for_test.TempGitTree() as tempdir: