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

Should configuration macro be named OPENTELEMETRY_XXX instead of just XXX ? #3084

Open
Romain-Geissler opened this issue Oct 7, 2024 · 0 comments
Assignees
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@Romain-Geissler
Copy link

Romain-Geissler commented Oct 7, 2024

Hi,

In the context of a build using CMake (I definitely don't know about the other build systems), we end up with some macros being defined to build both opentelemetry itself and proprietary code depending on opentelemetry. For example at the end of the CMake build, we have in a generated CMake flags file (most likely a CMake implementation detail), something like this:

./sdk/src/common/CMakeFiles/opentelemetry_common.dir/flags.make:5:CXX_DEFINES = -DENABLE_ASYNC_EXPORT -DENABLE_OTLP_COMPRESSION_PREVIEW -DOPENTELEMETRY_ABI_VERSION_NO=1 -DOPENTELEMETRY_STL_VERSION=2017

And here we can see we have a mix of "namespaced" macros (ie OPENTELEMETRY_ABI_VERSION_NO) and "non-namespaced" ones (ie ENABLE_ASYNC_EXPORT).

In our company we got today a pull request of someone adding explicitly the -DENABLE_ASYNC_EXPORT compilation flag in our internal build system (which isn't CMake), and I argued that this macro name is far too generic, it should have some kind of OPENTELEMTRY_ prefix, to "namespace" it, and make it avoid any collision with any other open source or proprietary code.

Would you accept a pull request adding this prefix to these macros (and maybe more if we find more) ? Are these macros part of the "public API" ? I hope not :D

Thanks,
Romain

@Romain-Geissler Romain-Geissler added the bug Something isn't working label Oct 7, 2024
@github-actions github-actions bot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Oct 7, 2024
@marcalff marcalff added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Oct 7, 2024
@marcalff marcalff self-assigned this Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

2 participants