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

[19865] Add app_id to monitor participant #217

Merged
merged 5 commits into from
Dec 19, 2023

Conversation

jepemi
Copy link
Contributor

@jepemi jepemi commented Dec 15, 2023

Add two new arguments to init_monitor() methods to set app_id and app_metadata discovery properties. Default values are "UNKNOWN_APP" and an empty string respectively.

@jepemi jepemi force-pushed the feature/add_monitor_app_id_property branch from 525e415 to 20b3df9 Compare December 15, 2023 12:11
@jepemi jepemi changed the title [] Add app_id to monitor participant [19865] Add app_id to monitor participant Dec 15, 2023
Copy link
Contributor

@irenebm irenebm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, add this change to the InitMonitorTests tests

examples/cpp/HelloWorldExample/HelloWorldPublisher.cpp Outdated Show resolved Hide resolved
examples/cpp/HelloWorldExample/HelloWorldSubscriber.cpp Outdated Show resolved Hide resolved
@jepemi jepemi force-pushed the hotfix/status_warnings branch from 4263dd3 to 70a6e35 Compare December 18, 2023 10:40
@jepemi jepemi force-pushed the feature/add_monitor_app_id_property branch from 0fdf10a to 6e0de3c Compare December 18, 2023 11:35
@jepemi jepemi force-pushed the hotfix/status_warnings branch from ed1d34a to 3e87670 Compare December 18, 2023 13:36
Base automatically changed from hotfix/status_warnings to main December 18, 2023 15:34
@jepemi jepemi force-pushed the feature/add_monitor_app_id_property branch from 6e0de3c to 25f3c18 Compare December 18, 2023 15:42
Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (f8fb83f) 58.20% compared to head (7c90128) 58.16%.

❗ Current head 7c90128 differs from pull request most recent head 14dc455. Consider uploading reports for the commit 14dc455 to get more accurate results

Files Patch % Lines
src/cpp/StatisticsBackend.cpp 20.00% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #217      +/-   ##
==========================================
- Coverage   58.20%   58.16%   -0.05%     
==========================================
  Files          40       40              
  Lines        5678     5682       +4     
  Branches     2989     2993       +4     
==========================================
  Hits         3305     3305              
  Misses         88       88              
- Partials     2285     2289       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Jesus Perez <[email protected]>
@jepemi jepemi force-pushed the feature/add_monitor_app_id_property branch from 25f3c18 to a9bfe7d Compare December 19, 2023 06:42
Signed-off-by: Jesus Perez <[email protected]>
irenebm
irenebm previously approved these changes Dec 19, 2023
Copy link
Contributor

@irenebm irenebm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Jesus Perez <[email protected]>
@rsanchez15 rsanchez15 merged commit af64033 into main Dec 19, 2023
9 of 10 checks passed
@rsanchez15 rsanchez15 deleted the feature/add_monitor_app_id_property branch December 19, 2023 08:42
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