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

Create generic tedge log plugin #2086

Merged
merged 4 commits into from
Aug 18, 2023
Merged

Conversation

Ruadhri17
Copy link
Contributor

@Ruadhri17 Ruadhri17 commented Jul 20, 2023

Proposed changes

to do:

  • Create tedge_log_plugin
  • Replace hardcoded values of topic root and topic identifier with tedge config values

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

#2048 and #2017

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

@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request July 20, 2023 18:23 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jul 20, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass %
251 0 5 251 100

@didier-wenzek
Copy link
Contributor

Since the plan is to deprecate c8y_log_plugin in favor of tedge_log_plugin, I would consider to start these changes with a git mv c8y_log_manager tedge_log_manager. This would help to focus on what is really new. The drawback is that this work will have then a larger scope including c8y-mapper updates and full deprecation of c8y-log-plugin.

This has to be discussed with @rina23q and @reubenmiller

@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #2086 (23f0fbd) into main (4ea00b6) will increase coverage by 0.0%.
The diff coverage is 79.2%.

❗ Current head 23f0fbd differs from pull request most recent head f5e2519. Consider uploading reports for the commit f5e2519 to get more accurate results

Additional details and impacted files
Files Changed Coverage Δ
crates/common/log_manager/src/error.rs 0.0% <ø> (ø)
.../tedge_config/src/tedge_config_cli/tedge_config.rs 87.2% <0.0%> (-1.3%) ⬇️
crates/extensions/c8y_log_manager/src/config.rs 0.0% <ø> (-47.4%) ⬇️
crates/extensions/c8y_log_manager/src/lib.rs 88.7% <ø> (ø)
crates/extensions/tedge_log_manager/src/config.rs 0.0% <0.0%> (ø)
crates/extensions/tedge_log_manager/src/error.rs 0.0% <0.0%> (ø)
plugins/tedge_log_plugin/src/main.rs 0.0% <0.0%> (ø)
crates/extensions/tedge_log_manager/src/actor.rs 75.9% <75.9%> (ø)
crates/common/log_manager/src/config.rs 76.5% <76.5%> (ø)
crates/extensions/c8y_log_manager/src/actor.rs 76.5% <84.6%> (-4.2%) ⬇️
... and 4 more

... and 2 files with indirect coverage changes

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.

I did a quick sanity review.

crates/common/log_manager/Cargo.toml Outdated Show resolved Hide resolved
crates/common/log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/common/log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/Cargo.toml Outdated Show resolved Hide resolved
@Ruadhri17 Ruadhri17 force-pushed the log-plugin-ref branch 4 times, most recently from 9e4e7e4 to accff1a Compare July 31, 2023 18:39
@rina23q rina23q had a problem deploying to Test Pull Request August 3, 2023 14:08 — with GitHub Actions Failure
@Ruadhri17 Ruadhri17 force-pushed the log-plugin-ref branch 12 times, most recently from c0d6c10 to a3f7af0 Compare August 7, 2023 08:42
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request August 8, 2023 12:16 — with GitHub Actions Inactive
@Ruadhri17 Ruadhri17 force-pushed the log-plugin-ref branch 4 times, most recently from 35b0494 to 95929f1 Compare August 8, 2023 15:27
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 job! Some unit tests must be moved and cleaned up to the right place!

crates/extensions/c8y_log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/common/log_manager/src/log_utils.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/extensions/c8y_log_manager/src/actor.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/src/tests.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/src/tests.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/src/tests.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/src/tests.rs Outdated Show resolved Hide resolved
crates/extensions/tedge_log_manager/src/tests.rs Outdated Show resolved Hide resolved
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request August 10, 2023 09:36 — with GitHub Actions Inactive
crates/extensions/tedge_log_manager/src/actor.rs Outdated Show resolved Hide resolved
Comment on lines +13 to +15

#[error(transparent)]
FromChannelError(#[from] tedge_actors::ChannelError),
Copy link
Contributor

Choose a reason for hiding this comment

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

This error case must not be caught as a LogManagementError (send back to the cloud) but lead to a fatal actor error (as the actor peer has been dropped).

Suggested change
#[error(transparent)]
FromChannelError(#[from] tedge_actors::ChannelError),

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, the issue is deeper and caused by self.http_proxy.await_response(request).await? mixing channel errors with HTTP errors. This will have to be improved by in a follow-up task.

@Ruadhri17 Ruadhri17 force-pushed the log-plugin-ref branch 2 times, most recently from 23f0fbd to 9b36f1b Compare August 17, 2023 09:59
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request August 17, 2023 10:10 — with GitHub Actions Inactive
Comment on lines +104 to +109
} else {
error!(
"Received unexpected message on topic: {}",
message.topic.name
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see how this can happen (unless the MQTT actor is buggy). Indeed this actor only subscribes to self.config.logfile_request_topic.

plugins/tedge_log_plugin/Cargo.toml Outdated Show resolved Hide resolved
Copy link
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.

Approved.

Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
@Ruadhri17 Ruadhri17 temporarily deployed to Test Pull Request August 18, 2023 09:42 — with GitHub Actions Inactive
@didier-wenzek didier-wenzek merged commit 05cec39 into thin-edge:main Aug 18, 2023
@gligorisaev
Copy link
Contributor

QA has thoroughly checked the feature and here are the results:

  • Test for ticket exists in the test suite.
  • QA has tested the feature and it meets the required specifications.
  • Checked the documentation

Test Cases will be created as soon as Installer is existing, that ticket has than to be linked with this one.

@reubenmiller reubenmiller added the theme:troubleshooting Theme: Troubleshooting and remote control label Sep 12, 2023
@reubenmiller reubenmiller added this to the 0.13.0 milestone Sep 12, 2023
@reubenmiller
Copy link
Contributor

Tests are covered by this PR #2213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme:troubleshooting Theme: Troubleshooting and remote control
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants