[server] Improve logging / error messages of GitLab app and context parser #4747

corneliusludmann · 2021-07-08T10:59:45Z

Improves the logging resp. error messages in the GitLab app resp. context parser. This hopefully gives us more insights into issue #4333.

…arser

components/server/src/gitlab/gitlab-context-parser.ts

AlexTugarev · 2021-07-08T13:11:26Z

components/server/src/gitlab/gitlab-context-parser.ts

-                        break;
-                    }
+            if (GitLab.ApiError.is(possibleTags)) {
+                throw new Error(`GitLab ApiError on searching for possible tags for ${owner}/${repoName}/tree/${segments.join('/')}: ${possibleTags}`);


AlexTugarev · 2021-07-08T13:28:19Z

components/server/src/gitlab/gitlab-context-parser.ts

@@ -214,29 +214,31 @@ export class GitlabContextParser extends AbstractContextParser implements IConte
        const possibleBranches = await this.gitlabApi.run<GitLab.Branch[]>(user, async g => {


why so complicated?
there is https://docs.gitlab.com/ee/api/branches.html#get-single-repository-branch

The problem is, that we don't know what of the segments is part of the branch and what is part of the file tree.

For example this context URL:

https://gitlab.com/gp-test-group/gp-test-subgroup/gp-test-project-in-subgroup/-/tree/corneliusludmann/important-issue-1/test-dir-1

corneliusludmann/important-issue-1 is the branch and

test-dir-1 is the file path.

But we cannot know this having just the context URL. It could also be that:

corneliusludmann is the branch and

important-issue-1/test-dir-1 is the file path.

We don't know this until we check for all possible branches that start with the first segement.

I believe this is the root cause of the issue here. A lucky shot revealed that some/lot of projects are structuring their branches in feature/9999_something patterns.
In L.212 we're searching for ^feature branches, which in many cases will be a large set of branches. Unfortunately, there is pagination in the GL REST API unconsidered here at: https://docs.gitlab.com/ee/api/#pagination
So, we always see only some of the feature* branches (maybe just 20.)

That is an interesting thought! Indeed, pagination could be the problem here. 💡

@corneliusludmann thanks for the explanation. I forgot about this dilemma.
Still, in that case we need a retry strategy with get-single-repository-branch calls and path prefix' in in ascending order, i.e. call it for seg1, seg1/seg2, seg1/seg2/seg3, etc. This is still less expensive than filtering n search calls.

Can you post such a case, please? It doesn't sound right. We can file an issue for GitLab.

Can you post such a case, please? It doesn't sound right.

https://gitlab.com/gp-test-group/gp-test-subgroup/gp-test-project-in-subgroup/-/tree/corneliusludmann is one

We can file an issue for GitLab.

Exactly. I have it already on my to-do list.

GitLab issue: https://gitlab.com/gitlab-org/gitlab/-/issues/335867

🥇 Awesome! Thanks for taking care of this and providing details to GitLab!

quick note on the response 500.
I've seen spikes of request times in server's logs. instead of regular ~300ms it took ~5s, which after checking turned out to be the response 500.
the it was a show branch request for foo, where foo does not exist but foo/bar does. this appears to be a real bug in GitLab.

AlexTugarev · 2021-07-12T10:22:22Z

/werft run

👍 started the job as gitpod-build-clu-improve-logging-gitlab.16

AlexTugarev · 2021-07-12T11:35:22Z


  21 passing (1m)
  4 failing

  1) TestGitlabContextParser
       testTreeContext_tag_01:

      AssertionError: expected { Object (isFile, path, ...) } to have deep property 'refType' of 'tag', but got 'branch'
      + expected - actual

      -branch
      +tag
      
      at TestGitlabContextParser.<anonymous> (src/gitlab/gitlab-context-parser.spec.ts:175:36)
      at Generator.next (<anonymous>)
      at fulfilled (src/gitlab/gitlab-context-parser.spec.ts:19:58)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

  2) TestGitlabContextParser
       testTreeContextSubgroup_tag_01:

      AssertionError: expected { Object (isFile, path, ...) } to have deep property 'refType' of 'tag', but got 'branch'
      + expected - actual

      -branch
      +tag
      
      at TestGitlabContextParser.<anonymous> (src/gitlab/gitlab-context-parser.spec.ts:423:36)
      at Generator.next (<anonymous>)
      at fulfilled (src/gitlab/gitlab-context-parser.spec.ts:19:58)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

  3) TestGitlabContextParser
       testTreeContextTagWithSlash:

      AssertionError: expected { Object (isFile, path, ...) } to have deep property 'refType' of 'tag', but got 'branch'
      + expected - actual

      -branch
      +tag
      
      at TestGitlabContextParser.<anonymous> (src/gitlab/gitlab-context-parser.spec.ts:507:36)
      at Generator.next (<anonymous>)
      at fulfilled (src/gitlab/gitlab-context-parser.spec.ts:19:58)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

  4) TestGitlabContextParser
       testEmptyProject:
     AssertionError: expected { Object (isFile, path, ...) } to have deep property 'ref'
      at TestGitlabContextParser.<anonymous> (src/gitlab/gitlab-context-parser.spec.ts:548:36)
      at Generator.next (<anonymous>)
      at fulfilled (src/gitlab/gitlab-context-parser.spec.ts:19:58)
      at runMicrotasks (<anonymous>)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

@corneliusludmann this change breaks context parser tests, unfortunately.

AlexTugarev · 2021-07-12T11:37:07Z

@corneliusludmann, you can run the tests with

cd components/server
export export GITPOD_TEST_TOKEN_GITLAB="{ \"value\": \"YOURTOKEN\", \"username\": \"oauth2\", \"scopes\": [ \"api\", \"read_user\" ] }"
yarn test

corneliusludmann · 2021-07-12T11:48:19Z

Ah, it skips it when the env var is not set. That explains why it didn't fail for me. 🙄 Thanks for the hint.

Try the conjunction of all segments as branch/tag name first before searching for parts of the branch/tag. That hopefully decreases the number of needed API calls.

corneliusludmann · 2021-07-12T15:32:55Z

Stupid me. Sorry for not recognizing this and thanks for carefully reviewing the PR, @AlexTugarev. 👍

I've fixed this and updated the PR.

There is still a failing test but this test fails on main as well. I've created the issue #4773 to fix this in the course of groundwork.

AlexTugarev · 2021-07-13T05:23:21Z

/werft run

👍 started the job as gitpod-build-clu-improve-logging-gitlab.18

AlexTugarev

'Tested with positive and negative examples and it worked nicely!

Thanks for fixing this, @corneliusludmann!

🎉 🎉 🎉

[server] Improve logging / error messages of GitLab app and context p…

e2439d8

…arser

corneliusludmann requested review from a team, geropl and AlexTugarev and removed request for a team and geropl July 8, 2021 10:59

AlexTugarev reviewed Jul 8, 2021

View reviewed changes

components/server/src/gitlab/gitlab-context-parser.ts Outdated Show resolved Hide resolved

AlexTugarev reviewed Jul 8, 2021

View reviewed changes

corneliusludmann marked this pull request as draft July 8, 2021 15:05

corneliusludmann force-pushed the clu/improve-logging-gitlab branch 6 times, most recently from 5adc4ee to 7785a6c Compare July 8, 2021 19:51

corneliusludmann marked this pull request as ready for review July 9, 2021 08:11

corneliusludmann requested a review from AlexTugarev July 9, 2021 08:11

corneliusludmann force-pushed the clu/improve-logging-gitlab branch 6 times, most recently from 1c635db to f769375 Compare July 9, 2021 15:07

[server] Improve branch/tag API call in GitLab context parser

36ff6c2

Try the conjunction of all segments as branch/tag name first before searching for parts of the branch/tag. That hopefully decreases the number of needed API calls.

corneliusludmann force-pushed the clu/improve-logging-gitlab branch from f769375 to 36ff6c2 Compare July 12, 2021 14:48

AlexTugarev approved these changes Jul 13, 2021

View reviewed changes

corneliusludmann merged commit 974ce0a into main Jul 13, 2021

corneliusludmann deleted the clu/improve-logging-gitlab branch July 13, 2021 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[server] Improve logging / error messages of GitLab app and context parser #4747

[server] Improve logging / error messages of GitLab app and context parser #4747

corneliusludmann commented Jul 8, 2021

AlexTugarev Jul 8, 2021

AlexTugarev Jul 8, 2021

corneliusludmann Jul 8, 2021

AlexTugarev Jul 8, 2021

corneliusludmann Jul 8, 2021

AlexTugarev Jul 8, 2021

AlexTugarev Jul 12, 2021

corneliusludmann Jul 12, 2021

corneliusludmann Jul 12, 2021

AlexTugarev Jul 13, 2021

AlexTugarev Jul 13, 2021

AlexTugarev commented Jul 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev commented Jul 12, 2021

AlexTugarev commented Jul 12, 2021

corneliusludmann commented Jul 12, 2021

corneliusludmann commented Jul 12, 2021

AlexTugarev commented Jul 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev left a comment

		@@ -214,29 +214,31 @@ export class GitlabContextParser extends AbstractContextParser implements IConte
		const possibleBranches = await this.gitlabApi.run<GitLab.Branch[]>(user, async g => {

[server] Improve logging / error messages of GitLab app and context parser #4747

[server] Improve logging / error messages of GitLab app and context parser #4747

Conversation

corneliusludmann commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTugarev commented Jul 12, 2021 • edited by werft-gitpod-dev-com bot Loading

AlexTugarev commented Jul 12, 2021

AlexTugarev commented Jul 12, 2021

corneliusludmann commented Jul 12, 2021

corneliusludmann commented Jul 12, 2021

AlexTugarev commented Jul 13, 2021 • edited by werft-gitpod-dev-com bot Loading

AlexTugarev left a comment

Choose a reason for hiding this comment

AlexTugarev commented Jul 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

AlexTugarev commented Jul 13, 2021 •

edited by werft-gitpod-dev-com bot

Loading