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

Fix cmake find config of fluentd and update corresponding README #478

Merged
merged 1 commit into from
Aug 29, 2024
Merged

Conversation

zurex
Copy link
Contributor

@zurex zurex commented Aug 29, 2024

Description

This PR raised for address issue #477, navigate to the issue to get more details.

Changes

  • Update the exporters/fluentd/cmake/opentelemetry-cpp-fluentd-config.cmake.in to make sure it works as expected.
  • Add new section Incorporating into an existing CMake Project in the README of fluentd for better guide.

Test && Validation

  • Test in local linux system and can't provide a unitest (at least i dont know the correct way).

Existing Behavior

Use find_package(opentelemetry-fluentd CONFIG REQUIRED) get empty OPENTELEMETRY_CPP_FLUENTD_LIBRARY_DIRS and OPENTELEMETRY_CPP_FLUENTD_INCLUDE_DIRS

New Behavior

Now OPENTELEMETRY_CPP_FLUENTD_LIBRARY_DIRS and OPENTELEMETRY_CPP_FLUENTD_INCLUDE_DIRS works as expected:

  • OPENTELEMETRY_CPP_FLUENTD_LIBRARY_DIRS: [opentelemetry-fluentd::trace, opentelemetry-fluentd::logs]
  • OPENTELEMETRY_CPP_FLUENTD_INCLUDE_DIRS: same as the value of OPENTELEMETRY_CPP_INCLUDE_DIRS

@zurex zurex requested a review from a team August 29, 2024 11:49
Copy link

linux-foundation-easycla bot commented Aug 29, 2024

CLA Signed


The committers listed above are authorized under a signed CLA.

@lalitb lalitb merged commit 1bff200 into open-telemetry:main Aug 29, 2024
2 checks passed
@ThomsonTan
Copy link
Contributor

I am not sure opentelelemetry-fluentd is a good name here, because it is a fluentd exporter for opentelemetry-cpp.

@lalitb
Copy link
Member

lalitb commented Aug 29, 2024

I believe target name is coming from project-name defined in CMakeLists.txt, and not defined in this PR. This PR just fixes that name to be properly used in OPENTELEMETRY_CPP_FLUENTD_LIBRARY_DIRS.

But agree, opentelemetry-cpp-fluentd is better naming, we can have separate PR to fix that.

@zurex
Copy link
Contributor Author

zurex commented Sep 2, 2024

@ThomsonTan as lalitb mentioned, this PR only do a bug fix and make sure the find_package could works as expected. If you want, i could create a separate PR to make sure the project name follow the same name pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants