CMake Telemetry Modules

[steve-downey]

Configuration and callback scripts for using cmake_instrumentation.

[ednolan]

I'd prefer to have the following changes made before we merge this in:

• Because we vendor infra/, including docs/tracing_example.png means we automatically add half a megabyte to the size of all beman repositories; not to mention that if we change the image, the git history needs to include both of them. I don't think we need the image for users to understand what this does.

• The word "telemetry" can have a connotation, not intended here, of software that collects some information and then "phones home," like Firefox telemetry. I'd rather call this "BuildProfiling" or "BuildTimeProfiling" or "BuildPerformanceProfiling" than "BuildTelemetry".

• Ever since bemanproject/exemplar@df40a6d, we've been bringing in beman_install_library via a direct include() of the vendored file, rather than with find_package() and -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES. As such, I think we should remove BuildTelemetry.cmake and adjust the documentation in infra to say:

  include(infra/cmake/BuildTelemetryConfig.cmake)
  configure_build_telemetry()

  rather than

  find_package(BuildTelemetry)
  configure_build_telemetry()

  I'm aware that find_package is more of a CMake best practice, but I'm of the opinion that maybe we can transition to using it over direct includes if we convert infra to a proper package-managed package rather than a vendored directory, and as long as it's vendored it doesn't make sense to impose the burden of adding -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES; also, it shouldn't be inconsistent with beman-install-library.cmake.

  I know that this kills the ability to toggle enabling/disabling telemetry by choosing whether to specify the -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES, but since Beman is a project where all the libraries are based on a project template, and we can now bring them all up to date with the latest copy of that template, there's no reason not to just have every library have a call to configure_build_telemetry() in its top level CMakeLists.txt.

Other than those items, looks good.

[steve-downey]

I'd prefer to have the following changes made before we merge this in:

• Because we vendor infra/, including docs/tracing_example.png means we automatically add half a megabyte to the size of all beman repositories; not to mention that if we change the image, the git history needs to include both of them. I don't think we need the image for users to understand what this does.

I've explained this to a few hundred people in the last couple months. Most of them needed pictures. But, I agree, thinking about it, they don't all need the picture in repo. Maybe stash it in a wiki here, or in bemanproject/beman or somewhere else.

• The word "telemetry" can have a connotation, not intended here, of software that collects some information and then "phones home," like Firefox telemetry. I'd rather call this "BuildProfiling" or "BuildTimeProfiling" or "BuildPerformanceProfiling" than "BuildTelemetry".

Naming is hard, and instrumentation also has a bunch of issues. Telemetry is one of the actual use cases for the ~15K CMake projects I'm applying this to now. Although I don't expect it to change much, I'd like to not have to have this fork drift too far from what I've got going into production to monitor integration build perfomance.

• Ever since bemanproject/exemplar@df40a6d, we've been bringing in beman_install_library via a direct include() of the vendored file, rather than with find_package() and -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES. As such, I think we should remove BuildTelemetry.cmake and adjust the documentation in infra to say:

  include(infra/cmake/BuildTelemetryConfig.cmake)
  configure_build_telemetry()

We can doc this either way, but as an actual external reused module is how it is being rolled out. Vendoring in and modifying many many packages was a non-starter. The two mechanisms are entirely equivalent.
I'd like to have a better solution for packaging. Although for the loose coupling we have and the rate of change, vendoring in means we don't break everything at once.

rather than

find_package(BuildTelemetry)
configure_build_telemetry()

I'm aware that find_package is more of a CMake best practice, but I'm of the opinion that maybe we can transition to using it over direct includes if we convert infra to a proper package-managed package rather than a vendored directory, and as long as it's vendored it doesn't make sense to impose the burden of adding -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES; also, it shouldn't be inconsistent with beman-install-library.cmake.

You can have both. The include form right now pulls in the same code as the find_package form. It's not one or the other, it's either and both.

include(infra/cmake/BuildTelemetry.cmake)

or

find_package(BuiltTelemetry)

or

-DCMAKE_PROJECT_TOP_LEVEL_INCLUDES=BuildTelemetry.cmake

all do the same thing, and compose to make sure they are only done once in any case. Tests are there to check that instrumentation does not get enabled twice.

I know that this kills the ability to toggle enabling/disabling telemetry by choosing whether to specify the -DCMAKE_PROJECT_TOP_LEVEL_INCLUDES, but since Beman is a project where all the libraries are based on a project template, and we can now bring them all up to date with the latest copy of that template, there's no reason not to just have every library have a call to configure_build_telemetry() in its top level CMakeLists.txt.

Good news/Bad news -- I've started using infra outside Beman?

https://github.com/steve-downey/example

and

https://github.com/steve-downey/surround where example needs to be pushed back into soon.

beman_install_library is a big win, and the actions.

[steve-downey]

Rebased on main