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

Adding LDMS Connector/Forwarder #518

Merged
merged 12 commits into from
Dec 8, 2023
Merged

Adding LDMS Connector/Forwarder #518

merged 12 commits into from
Dec 8, 2023

Conversation

vsurjadidjaja
Copy link
Contributor

Adding the LDMS connector/forwarder and all necessary support files.

Adding LDMS calls to CMakeLists
Adding the Caliper-LDMS connector/forwarder and CMakeLists files to services directory.
Adding LDMS Libraries to CMakeList to allow cali-query to identify LDMS publish functions.
@daboehme daboehme self-assigned this Nov 28, 2023
@daboehme
Copy link
Member

daboehme commented Nov 28, 2023

Thanks for the PR @vsurjadidjaja !

I've noticed a few things:

  • Can you rebase your branch with the current master? Some things in the top-level CMakeLists.txt got reverted in the PR, such as the version number
  • Did you write a FindLDMS.cmake file? It's missing in the PR
  • It would be better to use Caliper's log mechanism instead of printf in the code. It works like Log(0).stream() << "log message\n". The number indicates the log level, where 0 is a critical error, 1 is informational, and 2 is debug level. You won't see level 1 and 2 output unless you set the CALI_LOG_VERBOSITY environment variable to 1 or 2, respectively.
  • Not sure why we explicitly need to add the ldms dependency to cali-query - is ldms a static library?

Taking out unnecessary LDMS lib link
Adding LDMS path to README instructions.
This avoids seg faulting the program if user's do not set LDMS's env variables.
@vsurjadidjaja
Copy link
Contributor Author

Hi @daboehme!!

Thank you for taking a looking at my PR. I've rebased my fork to upstream/main. Please let me know if that's correct. I've also changed all messages to Caliper's logging format. To resolve the change I made in cali-query's CMakeLists.txt, I added two env variables in the README that instructs users to set the LD_LIBRARY_PATH and PATH so that Caliper can find LDMS's lib, bin, and sbin based on where the user installs LDMS.

@daboehme
Copy link
Member

daboehme commented Dec 8, 2023

Hi @vsurjadidjaja , thanks again for the PR! I fixed a few things in the top-level CMakeLists.txt that were still somehow reverting some changes in the main branch, but otherwise it looks good to me! I'll merge it in. I'll try to find a machine with LDMS here where I can give this a spin.

@daboehme daboehme merged commit ecd9138 into LLNL:master Dec 8, 2023
1 check passed
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.

None yet

2 participants