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

Improve reliability and performance when using the autobuild build mode #2235

Merged
merged 12 commits into from
Apr 16, 2024

Conversation

henrymercer
Copy link
Contributor

@henrymercer henrymercer commented Apr 12, 2024

This PR directly traces autobuild scripts when a build mode is specified to the init Action.

Before, the init Action did not know whether the workflow would call the autobuilder or run manual build scripts, so we always had to start indirect tracing in case we needed to trace manual build steps.

However in new-style workflows where the build mode is specified, this is no longer necessary, and we only need to use indirect tracing when the build mode is manual. For the autobuild build mode, we can avoid starting indirect tracing and directly trace the autobuild script using database trace-command. We expect this to provide reliability and performance benefits, which we will validate.

This is behind the autobuild_direct_tracing_enabled feature flag.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@henrymercer henrymercer force-pushed the henrymercer/autobuild-with-direct-tracing branch from 8859bb4 to 2eaad47 Compare April 12, 2024 16:07
@henrymercer henrymercer changed the base branch from main to henrymercer/feature-flags-with-tool-feature-dependencies April 12, 2024 16:07
@henrymercer henrymercer marked this pull request as ready for review April 12, 2024 17:00
@henrymercer henrymercer requested a review from a team as a code owner April 12, 2024 17:00
Base automatically changed from henrymercer/feature-flags-with-tool-feature-dependencies to main April 12, 2024 17:00
@henrymercer henrymercer changed the title Improve the reliability and performance of analyzing code when using the autobuild build mode Improve reliability and performance when using the autobuild build mode Apr 12, 2024
Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

Looks great.

CHANGELOG.md Outdated
@@ -16,6 +16,7 @@ Note that the only difference between `v2` and `v3` of the CodeQL Action is the
We recommend removing any references to these from your workflows. For more information, see the release notes for CodeQL Action v3.23.0 and v2.23.0.
- Automatically overwrite an existing database if found on the filesystem. [#2229](https://github.com/github/codeql-action/pull/2229)
- Bump the minimum CodeQL bundle version to 2.12.6. [#2232](https://github.com/github/codeql-action/pull/2232)
- We are rolling out a feature in April 2024 that improves the reliability and performance of analyzing code when using the `autobuild` build mode. This feature uses direct tracing to trace the build when the `autobuild` build mode is specified using the `build-mode` input to the `init` Action. [#2335](https://github.com/github/codeql-action/pull/2335)
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a lot going on in this changelog. Can you add some links to external docs around direct tracing. Also, is there a github changelog post for the build-mode changes you can link to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point — I'll leave the details out of the changelog note, and link some docs about build modes.

@@ -0,0 +1,30 @@
name: "Autobuild direct tracing"
description: "An end-to-end integration test of a Java repository built using 'build-mode: autobuild', with direct tracing enabled"
operatingSystems: ["ubuntu"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be testing on windows, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think this makes sense. It doesn't really give us any additional confidence that the code changes are correct, but it does give us better confidence that direct tracing is working as expected.

name: "Autobuild direct tracing"
description: "An end-to-end integration test of a Java repository built using 'build-mode: autobuild', with direct tracing enabled"
operatingSystems: ["ubuntu"]
versions: ["latest", "nightly-latest"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something to fix in this PR, but how can we ensure that as we release new versions of the CLI, we add them to the matrix? There are other integration tests that only run on "latest", "nightly-latest" because when we wrote them, those were the only versions that supported the feature being tested.

Could we add a minimum-version property so that tests that specify it run on all CLI versions >= the minimum version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this idea, but I think it's beyond the scope of this PR. I'll file an issue.

Comment on lines +674 to +676
if (config.buildMode === BuildMode.Autobuild) {
applyAutobuildAzurePipelinesTimeoutFix();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand why're now applying this fix only if we're using autobuild/direct tracing 🤔 it looks like we set those options unconditionally in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this PR should be additive when it comes to applying this fix — previously we applied it to the autobuild Action only, now we also apply it to database trace-command calls.

@henrymercer henrymercer self-assigned this Apr 15, 2024
angelapwen
angelapwen previously approved these changes Apr 15, 2024
CHANGELOG.md Outdated
@@ -16,7 +16,7 @@ Note that the only difference between `v2` and `v3` of the CodeQL Action is the
We recommend removing any references to these from your workflows. For more information, see the release notes for CodeQL Action v3.23.0 and v2.23.0.
- Automatically overwrite an existing database if found on the filesystem. [#2229](https://github.com/github/codeql-action/pull/2229)
- Bump the minimum CodeQL bundle version to 2.12.6. [#2232](https://github.com/github/codeql-action/pull/2232)
- We are rolling out a feature in April 2024 that improves the reliability and performance of analyzing code when using the `autobuild` build mode. This feature uses direct tracing to trace the build when the `autobuild` build mode is specified using the `build-mode` input to the `init` Action. [#2335](https://github.com/github/codeql-action/pull/2335)
- We are rolling out a feature in April/May 2024 that improves the reliability and performance of analyzing code when analyzing a compiled language with the `autobuild` [build mode](https://docs.github.com/en/code-security/code-scanning/creating-an-advanced-setup-for-code-scanning/codeql-code-scanning-for-compiled-languages#codeql-build-modes). [#2335](https://github.com/github/codeql-action/pull/2335)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed PR number should be 2235 instead of 2335 😸

@henrymercer henrymercer merged commit 18111b6 into main Apr 16, 2024
320 checks passed
@henrymercer henrymercer deleted the henrymercer/autobuild-with-direct-tracing branch April 16, 2024 17:59
@github-actions github-actions bot mentioned this pull request Apr 17, 2024
8 tasks
@github-actions github-actions bot mentioned this pull request Apr 17, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants