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

Health status documentation #2375

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

Bravo555
Copy link
Contributor

Proposed changes

This PR updates the markdown documentation regarding health statuses and health checks.

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

@Bravo555 Bravo555 force-pushed the docs/2348/health-documentation branch 2 times, most recently from 5fd1107 to f921d95 Compare October 27, 2023 08:27
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
344 0 4 344 100 53m22.164s

@Bravo555 Bravo555 marked this pull request as ready for review October 27, 2023 09:49
@Bravo555 Bravo555 temporarily deployed to Test Pull Request October 27, 2023 09:50 — with GitHub Actions Inactive
@reubenmiller reubenmiller mentioned this pull request Oct 28, 2023
11 tasks
Copy link
Member

@rina23q rina23q left a comment

Choose a reason for hiding this comment

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

Good work! A bit more needs to be updated

docs/src/operate/troubleshooting/monitor_tedge_health.md Outdated Show resolved Hide resolved
docs/src/operate/troubleshooting/monitor_tedge_health.md Outdated Show resolved Hide resolved
@Bravo555 Bravo555 force-pushed the docs/2348/health-documentation branch from 5b800ec to 99ce3cb Compare October 30, 2023 14:47
@Bravo555 Bravo555 force-pushed the docs/2348/health-documentation branch from 99ce3cb to bb98d1a Compare October 30, 2023 14:48
@Bravo555 Bravo555 temporarily deployed to Test Pull Request October 30, 2023 14:59 — with GitHub Actions Inactive
@Bravo555 Bravo555 merged commit 928f43e into thin-edge:main Oct 31, 2023
16 checks passed
@Bravo555 Bravo555 deleted the docs/2348/health-documentation branch October 31, 2023 12:07
Copy link
Contributor

@albinsuresh albinsuresh left a comment

Choose a reason for hiding this comment

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

Sorry, was a bit late to submit my review. But there are some mistakes in the updated docs, that you'd have to correct.

docs/src/references/mappers/c8y-mapper.md Show resolved Hide resolved
|root|Base topic to group the identifier and channel under one common namespace|
|identifier|A descriptor which represents which device/service the channel data is related to|
|channel|Represents the information, such as telemetry data and commands, related to the **identifier**. Each channel type defines its own sub topic structure and corresponding payload format.|
| Group | Description |
Copy link
Contributor

Choose a reason for hiding this comment

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

Although I agree that this looks better, we're not sure if these reformattings are sustainable. Every time we add a new entry or update an existing entry, like Description, a similar reformatting will be required affecting all the lines. That's why we went for the easiest, maintainable option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I'll disable my markdown formatting extensions and try to not reformat stuff in the future.

docs/src/references/mqtt-api.md Show resolved Hide resolved
| a | Alarms |
| cmd | Commands |
| twin | Entity twin metadata |
| status | Service status |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| status | Service status |
| status | Entity status |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK currently we have only services publishing health status messages, and this status category is not clearly defined:

  • can health status messages be published for devices as well?
  • are there any other statuses valid for devices but not services?

I think this status category is only referenced here, so I'd rather be conservative with the wording here and only describe what is currently implemented, instead of trying to cover how it might be implemented in the future.

|`pid`|Process ID of the service|
|`status`|Service status. Possible values are `up` or `down`|
|`time`|Unix timestamp in seconds|
<!-- TODO: this should be in a reference about health status messages -->
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See this comment: #2375 (comment)

IMO we have a lot of repetition going on between different documentation pages, where a lot of the properties of different messages are mentioned in different contexts, and we should be aiming to have a reference describing these things in a single place.
Here, I don't think this table of properties of a health message doesn't add much value, as we don't refer to pid or time much in the rest of this document, but it's tricky where to draw the line which things are better to be extracted, and which be duplicated for the convenience of the reader.

Bravo555 added a commit to Bravo555/thin-edge.io that referenced this pull request Oct 31, 2023
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

6 participants