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

Refactor development/CI tooling #2609

Merged
merged 6 commits into from Feb 15, 2022
Merged

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Feb 14, 2022

  • Refactor Makefile to allow parallel target execution for independent Go modules. This allows existing behavior, serial target code execution, to continue when running make with only one job (default). However, it enables local development, where multiple CPUs are available, to parallelize with multiple jobs.
  • Clean up target variable value generation.
    • The ALL_GO_MOD_DIRS variable no longer filters out examples and then adds them back in.
    • The ALL_GO_MOD_DIRS variable includes all Go module directories, including the tools module.
    • The OTEL_GO_MOD_DIRS is added to filter the tools module out of ALL_GO_MOD_DIRS.
    • Remove the EXAMPLES variable, it is no longer used (see below).
  • The build of the examples and the rest of the project directories have been unified into the single build target. The old examples target is removed. It was always run on the CI system after the build target anyway.
  • The default test target, test-default, no longer outputs verbosely. Any relevant test failure will include a verbose output, but having it as the default creates human unreadable output.
  • The dependency order of crosslink and running go mod tidy for modules is reversed. All modules are crosslinked before tidying to prevent "unknown module errors"
  • A new golangci-lint and golangci-lint-fix target are split from lint to run golangci-lint dirctly. The golangci-lint target is split to be a dependency of the ci target, and the golangci-lint-fix target is a dependency of precommit (see below on distinction). These can be run in parallel for jobs > 1.
  • A new go-mod-tidy target is split from lint-modules to run go mod tidy in every module directory. This can be run in parallel for jobs > 1.
  • The ci no longer depends on precommit. The ci target is trimmed down to only check things (instead of fixing things and failing without any useful information about what the actual changes to the files are). The precommit target now will do it's best to fix any fixable issues it finds.
    • This speeds up both targets by not running golangci-lint twice (once with fix the other without), instead only running the appropriate command.

Local Performance Overview

There are not comprehensive benchmarks, but show the general performance benifits of running locally with 8 jobs on an 8 core machine.

$ #Branch: MrAlias:make-tooling
$ time (make -j8 &>/dev/null && echo $?)
  203.96s user 33.45s system 599% cpu 39.615 total
0

$ #Branch: main
$ time (make -j8 &>/dev/null && echo $?)
  253.43s user 51.33s system 510% cpu 59.684 total
0

Which represents ~30% improvement for the experience runtime.

@MrAlias MrAlias added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 14, 2022
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #2609 (af6720f) into main (f5c4874) will decrease coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##            main   #2609     +/-   ##
=======================================
- Coverage   76.2%   76.2%   -0.1%     
=======================================
  Files        173     173             
  Lines      12238   12238             
=======================================
- Hits        9328    9326      -2     
- Misses      2667    2669      +2     
  Partials     243     243             
Impacted Files Coverage Δ
exporters/jaeger/jaeger.go 92.6% <0.0%> (-0.9%) ⬇️

Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

🤩

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

:shipit:

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@MrAlias MrAlias merged commit cd21df4 into open-telemetry:main Feb 15, 2022
@MrAlias MrAlias deleted the make-tooling branch February 15, 2022 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants