Remove unused header files and use forward declarations when necessary

[cngzhnp]

Fixes # (issue)

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added
• Changes in public API reviewed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: cngzhnp / name: Cengizhan Pasaoglu (b7a6295)

[marcalff]

@cngzhnp

Thanks for working on this, the code indeed needs some cleanup in this area.

Some early comments:

• is this a manual change, or based on some tooling like include-what-you-use ?
• use clang-format to format the code
• remember to sign the CLA
• make a local build to test the changes, with as many options enabled as possible, to catch any build break up front

[codecov]

Codecov Report

Merging #2043 (493ebef) into main (7ee86a9) will not change coverage.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2043   +/-   ##
=======================================
  Coverage   87.19%   87.19%           
=======================================
  Files         166      166           
  Lines        4784     4784           
=======================================
  Hits         4171     4171           
  Misses        613      613           

Impacted Files	Coverage Δ
api/include/opentelemetry/common/kv_properties.h	98.97% <ø> (ø)
api/include/opentelemetry/common/string_util.h	100.00% <ø> (ø)
api/include/opentelemetry/common/timestamp.h	80.44% <ø> (ø)
api/include/opentelemetry/context/context.h	100.00% <ø> (ø)
...lemetry/context/propagation/composite_propagator.h	100.00% <ø> (ø)
...pi/include/opentelemetry/context/runtime_context.h	97.60% <ø> (ø)
.../include/opentelemetry/metrics/async_instruments.h	100.00% <ø> (ø)
api/include/opentelemetry/metrics/meter.h	100.00% <ø> (ø)
api/include/opentelemetry/metrics/meter_provider.h	100.00% <ø> (ø)
...pi/include/opentelemetry/metrics/observer_result.h	100.00% <ø> (ø)
... and 82 more

[cngzhnp]

Hello @marcalff, thanks for your suggestions!

• TBH, I don't know any software tool that extract or detect forward declarations from source code. Of course, IWYU tool could help to find necessary include files in this manner, however still could not find the forward declarations.

• Before ending up all changes, I could run format tool again.

• I tried to build options that I could find but Is it possible to test all options at once in somewhere? Is there any script that I use in a local build?

• I will sign CLA afterwards my changes are done.

Also, there are too many changes at once (more than 100 files), maybe I should to split up into more PRs to review by you easily. Maybe, they should be like a component-based PR. What do you think?

[marcalff]

Hello @marcalff, thanks for your suggestions!

Hi @cngzhnp

The very first thing you should do is to NOT use force push when pushing a commit:

• this erases the entire PR history
• this erases existing review comments if any
• this erases the build logs available in CI

For clang-format, make sure to use the proper version, and check for format failures in CI.

See:

• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/docs/using-clang-format.md
• https://github.com/open-telemetry/opentelemetry-cpp/blob/main/tools/format.sh

[cngzhnp]

Hello @marcalff,

Thanks for your feedback but I do not see any rules on CONTRIBUTING.md file which does not contain any information about erasing PR history by using "force" command of Git when PR marked as draft.

IMHO, you are totally right if only if PR is ready for review by others. However, it should be depending on contributor itself if PR is in draft phase. After PR is marked as ready to review then every changes including review comments has to be kept. Maybe, CONTRIBUTING.md must be extended for developers more clearly. I am also looking forward your opinion about this topic.

Also, thanks for the hint, I need to change my clang-format version. However, it brings me another topic on the table: Docker image for build environment. Maybe, like you did on CI pipeline, we need to create another or same Docker image for developers who can contribute easily without touching any tools. For example; currently I am using Debian/bookworm version which is not marked as stable and gRPC version is different than you used in the pipeline. So, such kind of differences could come with unexpected behaviors. What is your opinion about that?

[marcalff]

Thanks for the general cleanup.

Please see comments, with details to address.

Given the patch size, it will probably be merged by parts.

[marcalff]

@cngzhnp Thanks for your work on this.

In this round, I limited my review to the content of api/ only,
with a minor comment on include new versus include exception in one file,
see detailed comments.

Consider the changes done in api/ approved by myself.

Please prepare a separate PR for the changes in api/ only,
so it can be merged independently.

After that, I will continue the review on the next parts (sdk, exporters, examples), in that order. The next parts are not ready (more comments to come), so not formally approving the whole PR yet.

@lalitb , @ThomsonTan , @esigo , @owent

The changes in api/ only look good to me (with 1 comment).

In particular:

• using forward declarations instead of include will help to clarify exact binary dependencies, when evaluating ABI changes (forward is better), so this cleanup is really needed, it is not just cosmetic.
• adding explicit includes, like version.h, follows the include-what-you-use concepts, and is also needed to have self contained headers, which also will help to identify explicitly all the dependencies from a given file, which will help for ABI checks.

When the PR for the content of api/ is posted by @cngzhnp , please review it as well, and merge api changes.

Do not merge this PR as a whole.

Thanks all.

[marcalff]

Should this be "include new" instead, for std::bad_alloc ?

[owent]

In modern compilers, we can use __cpp_exceptions to check if exception is enabled.
And use __EXCEPTIONS in legacy gcc, use __has_feature(cxx_exceptions) in legacy clang , use _CPPUNWIND in legacy MSVC.
Is is better to add a macro in opentelemetry/common/macros.h and use it here?
e.g.

#if defined(__cpp_exceptions)
#  define OPENTELEMETRY_HAVE_EXCEPTION __cpp_exceptions
#elif defined(__EXCEPTIONS) && __EXCEPTIONS
#  define OPENTELEMETRY_HAVE_EXCEPTION 1
#elif defined(__clang__)
#  if __has_feature(cxx_exceptions)
#    define OPENTELEMETRY_HAVE_EXCEPTION 1
#  endif
#elif defined(_MSC_VER)
#  if defined(_CPPUNWIND)
#    define OPENTELEMETRY_HAVE_EXCEPTION 1
#  endif
#endif