ci: Add dedicated CodeQL workflow for improved analysis

[thompson-tomo]

This adds a dedicated codeql workflow to resolve the issue with the current ci failures where in which codeql is tied to a non c++ project.

Nginix & httpd is excluded and will be enabled via #633 & #638

[github-advanced-security]

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

• The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
• Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
• You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

[dbarker]

Thanks for contributing to CI. It is needed! Just a request to use native CMake install for otel-cpp and apt packages for the dependencies so conan support is not implied and it is easier maintain.

[dbarker]

Please see below on steps to harden the workflow..

[dbarker]

Thanks for taking this on. Following the discussion above please remove the conanfiles and install the dependencies for the CodeQL test with either apt packages or using CMake's ExternalProject module.

[lalitb]

Thanks for adding the shared CodeQL workflow. Two concerns:

• I agree with @dbarker that CodeQL should not require adding Conan support to each component. Native CMake or system packages are a better fit here.
• The workflow triggers for all exporters/ and instrumentation/ changes, but only analyzes components with a Conan file. This can give a green CodeQL result for code that was not built. It also leaves the webserver module uncovered after ci: Resolving nginx & prometheus build issue + ci consistency #633 removes its existing CodeQL job.

Can we make the supported components explicit and ensure every triggered component is actually analyzed?

[thompson-tomo]

CodeQL should not require adding Conan support to each component. Native CMake or system packages are a better fit here.

Conan was used to only import dependencies hence we didn't add support for conan and they were still built using cmake.

The workflow triggers for all exporters/ and instrumentation/ changes, but only analyzes components with a Conan file. It also leaves the webserver module uncovered after ci #633 removes its existing CodeQL

The biggest difficulty we face is codeql requires everything to be built in the 1 workflow hence why we need a reproducible build process. The problem got even worse when you consider different components needed different versions of the same dependency as otherwise they failed. Hence the conan was added to the components which could be built with the goal being add more once they are buildable. Note prior to this pr there was no codeql at all as the current codeql doesn't work, hence partial coverage is better than none.

[lalitb]

The biggest difficulty we face is codeql requires everything to be built in the 1 workflow hence why we need a reproducible build process. The problem got even worse when you consider different components needed different versions of the same dependency as otherwise they failed. Hence the conan was added to the components which could be built with the goal being add more once they are buildable. Note prior to this pr there was no codeql at all as the current codeql doesn't work, hence partial coverage is better than none.

I agree that partial coverage is better than none. My concern is only that the workflow should clearly represent that partial coverage.

Right now, it triggers for every change under exporters/** and instrumentation/**, but it builds only the components that have a conanfile.txt. This means CodeQL can report green even when the changed component was not analyzed.

CodeQL also does not require every component to use the same build environment. Components with different dependency requirements can use separate jobs or a matrix.

Could we limit the path triggers to the components currently supported, or define those components explicitly in a matrix? We can expand that list as more components become buildable.

[thompson-tomo]

CodeQL also does not require every component to use the same build environment. Components with different dependency requirements can use separate jobs or a matrix.

I don't believe that is the case given that the build must be done between the init & analyze steps and the analyze replaces any previous analyze hence needs to be a single check.

I need to think of a maintainable & reproducible solution on how to perform codeql analysis especially if we can't use conan.

[thompson-tomo]

@dbarker after many, many attempts the ci is now failing after adding the fetch content rather than using conan. It has even broken the pure cmake build.

Note The codeql error is one of the errors i saw previously when using a newer version via conan but now i get the error on all fetch content versions.

[dbarker]

Thanks for the updates. The fetch content usage looks good. The compile errors are coming from some existing CMake issues in those files. They aren't linking the otel cpp API CMake target (this is also important to get the transitive compile definitions from the API target).

[dbarker]

OPENTELEMETRY_CPP_INCLUDE_DIRS isn't defined when using fetch content so this results in the headers not being found. That variable can be removed from the target_include_directories call.

The fix is to link opentelemetry-cpp::api to ${this_target} with target_link_libraries below.

[thompson-tomo]

That appears to not have worked as ci is still failing after said changes

[dbarker]

I've reviewed the errors and shared feedback in the follow-up review. Please see #634 (review)

[dbarker]

Please see feedback on the latest changes below.

[dbarker]

missing the public link to opentelemetry-cpp::api

target_link_libraries(${this_target} PUBLIC opentelemetry-cpp::api Boost::log)

[dbarker]

the glog sink exposes glog and opentelemetry-cpp headers publicly in its header. Both must be linked with PUBLIC scope.

target_link_libraries(${this_target} PUBLIC glog::glog opentelemetry-cpp::api)

[dbarker]

missing the link to opentelemetry-cpp::api. Links to the api and log4cxx must be with PUBLIC scope since the appender exposes headers for both publicly.

[dbarker]

${OPENTELEMETRY_CPP_INCLUDE_DIRS} can be removed once the api is properly linked. See below.

[dbarker]

Please remove the apt-packages.txt files from this PR.

The separate apt package files are not necessary for the purpose of running the CodeQL analysis, will introduce a maintenance burden, imply support for a package manager, and are unusual/unexpected for a C++ CMake project generally.

All dependencies needed to support analyzing the contrib components with CodeQL can be installed directly in one job step within the scope of the CodeQL workflow file. This will keep the PR well scoped and focused on enabling that CI workflow.

Please note several dependencies (ssl, curl, protobuf) may not be required by the CodeQL workflow and components tested.

[thompson-tomo]

This is purely a text file which is feed to apt-get so that each instrumentation/exporter has what it needs. Hence it doesn’t imply support for a package manager. Without it you are duplicating manually managed lists of what to install including hardcoding it in ci workflows. hence it is actually providing a pathway to reducing maintenance and that same time it serves as a marker of a package which can be built in a standardised manner.

The dependencies was based on what the testing workflows defined. My intention was to review them once the ci is building.

[dbarker]

The install error below indicates that either the CMake option to install log4cxx is not enabeld or the log4cxx CMake package does not support installation when built as a subproject.

The simple fix is to continue to install the package in the CI workflow files with apt and call find_package for log4cxx in this file.

CMake Error in CMakeLists.txt:
  export called with target "opentelemetry_log4cxx_appender" which requires
  target "log4cxx" that is not in any export set.


CMake Error in CMakeLists.txt:
  export called with target "opentelemetry_log4cxx_appender" which requires
  target "log4cxx" that is not in any export set.


CMake Generate step failed.  Build files cannot be regenerated correctly.

[thompson-tomo]

It is fair to say. i underestimated, the state of the repo and it's ability for the packages to follow a common build process to ensure a maintainable solution can be achieved so that codeql scanning can be performed. As such i have switched this to draft and instead raised #651 #652 & #653 to support this pr. The difference those pr's are focused on addressing the ci workflow and once implemented this pr should be automatically descoped as the issues attempting to be resolved here have already been tackled.