Skip to content

refactor: Refactor health status code#2919

Merged
albinsuresh merged 2 commits intothin-edge:mainfrom
albinsuresh:imp/cleanup-health-check
Jun 7, 2024
Merged

refactor: Refactor health status code#2919
albinsuresh merged 2 commits intothin-edge:mainfrom
albinsuresh:imp/cleanup-health-check

Conversation

@albinsuresh
Copy link
Copy Markdown
Contributor

@albinsuresh albinsuresh commented Jun 5, 2024 •
edited
Loading

Proposed changes

Refactor health status refactoring code, including addressing review comments from #2903

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

@albinsuresh albinsuresh temporarily deployed to Test Pull Request June 5, 2024 06:38 — with GitHub Actions Inactive
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jun 5, 2024 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
452 0 3 452 100 1h6m55.904892999s

didier-wenzek
didier-wenzek reviewed Jun 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

This PR is a nice step forward, grouping all health-check related logic in one place . However, some constants have still to be made private, in order to avoid inappropriate uses.

jarhodes314
jarhodes314 reviewed Jun 5, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@jarhodes314 jarhodes314 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm happy with the changes asides from the aforementioned pub const issues

@albinsuresh albinsuresh requested a review from reubenmiller as a code owner June 5, 2024 13:04
@albinsuresh albinsuresh temporarily deployed to Test Pull Request June 5, 2024 13:04 — with GitHub Actions Inactive
albinsuresh
albinsuresh commented Jun 5, 2024
View reviewed changes
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 79.73856% with 31 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (f186b3f) to head (4914cff).
Report is 3 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
crates/core/c8y_api/src/utils.rs 0.0% <ø> (-69.0%) ⬇️
crates/core/tedge_api/src/health.rs 95.8% <100.0%> (+3.1%) ⬆️
crates/core/tedge_api/src/mqtt_topics.rs 88.1% <ø> (+1.1%) ⬆️
crates/extensions/c8y_mapper_ext/src/actor.rs 82.3% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/converter.rs 83.7% <100.0%> (-0.1%) ⬇️
...s/extensions/c8y_mapper_ext/src/service_monitor.rs 97.7% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/tests.rs 92.6% <100.0%> (-0.1%) ⬇️
crates/extensions/tedge_mqtt_bridge/src/lib.rs 89.1% <100.0%> (ø)
crates/extensions/c8y_mapper_ext/src/config.rs 48.2% <87.5%> (+0.5%) ⬆️
crates/core/tedge_mapper/src/c8y/mapper.rs 0.0% <0.0%> (ø)
... and 3 more

... and 3 files with indirect coverage changes

didier-wenzek
didier-wenzek reviewed Jun 5, 2024
View reviewed changes
didier-wenzek
didier-wenzek reviewed Jun 5, 2024
View reviewed changes
crates/core/tedge_api/src/health.rs
mqtt_schema: &MqttSchema,
bridge_service_topic: &Topic,
) -> Result<Self, HealthTopicError> {
if let Ok((topic_id, Channel::Health)) = mqtt_schema.entity_channel_of(&message.topic) {
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek Jun 5, 2024

Choose a reason for hiding this comment

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

Unrelated to this PR, but this highlights an issue I got several time: this method uses a specific mqtt_schema only to parse a topic. The MQTT root prefix doesn't really matter and it would be more convenient to have a parse method returning all the component of a topic is set (i.e. not only the topic_id and channel but also the root prefix).

@albinsuresh albinsuresh temporarily deployed to Test Pull Request June 7, 2024 09:25 — with GitHub Actions Inactive
@albinsuresh albinsuresh requested a review from didier-wenzek June 7, 2024 10:03
didier-wenzek
didier-wenzek approved these changes Jun 7, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

This PR is a nice step forward, grouping all health-check related logic in one place .

@albinsuresh albinsuresh force-pushed the imp/cleanup-health-check branch from b30236a to 4914cff Compare June 7, 2024 13:03
@albinsuresh albinsuresh temporarily deployed to Test Pull Request June 7, 2024 13:03 — with GitHub Actions Inactive
@albinsuresh albinsuresh enabled auto-merge June 7, 2024 13:04
@albinsuresh albinsuresh added this pull request to the merge queue Jun 7, 2024
Merged via the queue into thin-edge:main with commit 4861be8 Jun 7, 2024
@albinsuresh albinsuresh deleted the imp/cleanup-health-check branch June 14, 2024 05:24
@didier-wenzek didier-wenzek mentioned this pull request Jul 11, 2024
Closed
@reubenmiller reubenmiller added the theme:monitoring Theme: Service monitoring and watchdogs label Aug 1, 2024
@reubenmiller reubenmiller mentioned this pull request Sep 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@didier-wenzek didier-wenzek didier-wenzek approved these changes

@rina23q rina23q Awaiting requested review from rina23q rina23q is a code owner

@reubenmiller reubenmiller Awaiting requested review from reubenmiller reubenmiller is a code owner

@jarhodes314 jarhodes314 Awaiting requested review from jarhodes314 jarhodes314 is a code owner

Assignees

@albinsuresh albinsuresh

Labels

theme:monitoring Theme: Service monitoring and watchdogs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@albinsuresh @didier-wenzek @jarhodes314 @reubenmiller