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

Config setting to disable auto-registration of entities #2490

Merged
merged 1 commit into from
Dec 4, 2023

Conversation

PradeepKiruvale
Copy link
Contributor

Proposed changes

Document configurable auto-registration

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

#2483

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

Copy link
Contributor

github-actions bot commented Nov 30, 2023

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
380 0 3 380 100 53m22.669999999s

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.

LGTM. Have given some rewording suggestions, though.

docs/src/references/mappers/c8y-mapper.md Outdated Show resolved Hide resolved
docs/src/references/mappers/c8y-mapper.md Outdated Show resolved Hide resolved
Comment on lines 223 to 225
The auto-registration can be enabled/disabled using the `tedge` tool.

The auto-registration can be disabled as below
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
The auto-registration can be enabled/disabled using the `tedge` tool.
The auto-registration can be disabled as below
The auto-registration can be disabled/enabled using the `tedge config` command as follows:

Comment on lines 236 to 247
The auto-registration can be enabled as below

```sh
sudo tedge config set c8y.entity_store.auto_register true
```
Or

```sh
sudo tedge config unset c8y.entity_store.auto_register
```
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
The auto-registration can be enabled as below
```sh
sudo tedge config set c8y.entity_store.auto_register true
```
Or
```sh
sudo tedge config unset c8y.entity_store.auto_register
```

I'd just drop that to reduce the verbosity, as the previous example is clear enough. Especially since it is just a boolean value with only two possible values.

Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems to be wrong. If the default is false then unset will disable auto registration. Isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default, the auto-registration is enabled. Here unset means to set the value to the default value.

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 default is true.

```sh
sudo tedge config unset c8y.entity_store.auto_register
```
:::note
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move this note immediately after the config setting example. It can even be merged with the previous comment, but as the very first statement.

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.

The content is here, but it could be clearer.

Comment on lines 236 to 247
The auto-registration can be enabled as below

```sh
sudo tedge config set c8y.entity_store.auto_register true
```
Or

```sh
sudo tedge config unset c8y.entity_store.auto_register
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Something seems to be wrong. If the default is false then unset will disable auto registration. Isn't it?

## Auto Registration of Entities

Before any data messages from an entity can be processed, the entity has to be registered first.
The entities can be registered either explicitly, or implicitly.
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
The entities can be registered either explicitly, or implicitly.
The entities can be registered either explicitly, or implicitly,
depending on the auto-registration mode set with `c8y.entity_store.auto_register`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 217 to 227
To register an entity explicitly, an entity registration message has to be sent.

But, if the auto-registration is enabled, sending any data message on a topic following the default topic scheme (`te/device/<device-id>/service/<service-id>/...`),
results in the auto-registration of that target entity.

For example, sending a measurement message to `te/device/child1///m/temperature`,
will result in the auto-registration of the entity with topic id: `device/child1//` and the auto-generated external id: `<main-device-id>:device:child1`, derived from the topic id.
Similarly, a measurement message on `te/device/child1/service/my-service/m/temperature`,
results in the auto-registration of both the device entity: `device/child1//` and the service entity: `device/child1/service/my-service` with their respective auto-generated external IDs, in that order.

The auto-registration can be disabled/enabled using the `tedge config` command as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

This text is difficult to read because going back and forth from auto-registration to explicit registration.

I would clearly separate the two:

The simpler is to use auto-registration which is the also the default behavior.
... all the details, pros and cons about auto-registration

Explicit registration allows to work around the limitations of auto-registration.
... how to turn it on, link to the doc on how to register an entity, what are the impacts (i.e. lost measurements till the appropriate registration is received).

Comment on lines 234 to 236
When the auto registration is disabled, and if the device is not already registered.
Then the c8y-mapper will not forward the incoming telemetry/health-status messages to the c8y cloud.
But, logs/publishes an error message on the `te/errors` topic telling the entity is not registered.
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
When the auto registration is disabled, and if the device is not already registered.
Then the c8y-mapper will not forward the incoming telemetry/health-status messages to the c8y cloud.
But, logs/publishes an error message on the `te/errors` topic telling the entity is not registered.
Auto-registration is enabled, by default.
When the auto registration is disabled, and if the device is not explicitly registered,
then the c8y-mapper will just ignore all the data messages received from that device,
logging that error message on the `te/errors` topic indicating that the entity is not registered.

Reasons for rewording:

  1. The first sentence was ending abruptly.
  2. The messages are not limited to telemetry and health-status messages.

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. This is really well written and clear. Thank you.

Signed-off-by: Pradeep Kumar K J <pkj@softwareag.com>
@PradeepKiruvale PradeepKiruvale merged commit 4363edf into thin-edge:main Dec 4, 2023
16 checks 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

3 participants