Skip to content

feat: publish properties along tedge measurements#3774

Merged
didier-wenzek merged 3 commits intothin-edge:mainfrom
didier-wenzek:fix/non-numeric-measurement-properties
Sep 10, 2025
Merged

feat: publish properties along tedge measurements#3774
didier-wenzek merged 3 commits intothin-edge:mainfrom
didier-wenzek:fix/non-numeric-measurement-properties

Conversation

@didier-wenzek
Copy link
Copy Markdown
Contributor

@didier-wenzek didier-wenzek commented Aug 28, 2025 •
edited
Loading

Proposed changes

Allow meta properties to be published along tedge measurements, and forward these properties unchanged to Cumulocity.

tedge mqtt pub te/device/main///m/example '{
  "time": "2020-10-15T05:30:47+00:00",
  "temperature": 25,
  "properties": {
    "sensor": "DS18B20",
    "has_alarm": false,
    "samples": [ 25.1, 25.0, 24.9],
    "range": {
       "min": "-55°C",
       "max": "+125°C"
    }
  }
}'

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

#3768

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
  • I ran just format as mentioned in CODING_GUIDELINES
  • I used just check 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

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 28, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 5.76923% with 49 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/extensions/c8y_mapper_ext/src/serializer.rs 0.00% 14 Missing ⚠️
crates/core/tedge_api/src/measurement/serialize.rs 15.38% 10 Missing and 1 partial ⚠️
crates/core/tedge_api/src/measurement/builder.rs 0.00% 7 Missing ⚠️
crates/core/tedge_api/src/measurement/group.rs 0.00% 7 Missing ⚠️
crates/common/json_writer/src/lib.rs 0.00% 6 Missing ⚠️
crates/core/tedge_api/src/measurement/parser.rs 20.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 28, 2025 •
edited
Loading

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
686 0 3 686 100 2h6m53.185032999s

@reubenmiller reubenmiller added the theme:telemetry Theme: Telemetry data label Aug 29, 2025
@didier-wenzek didier-wenzek marked this pull request as ready for review September 1, 2025 13:40
@didier-wenzek didier-wenzek force-pushed the fix/non-numeric-measurement-properties branch from 18c96f5 to 91dee11 Compare September 1, 2025 14:58
albinsuresh
albinsuresh reviewed Sep 3, 2025
View reviewed changes
Copy link
Copy Markdown
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.

The feature is working fine. Happy to approve once a few queries are clarified. Just noticed a few quirks:

  1. I could publish a measurement with only metadata and no measurement values as follows 
    tedge mqtt pub 'te/device/main///m/ac_temp' '{"unit": "C"}'
    which got converted to 
    [c8y/measurement/measurements/create] {"unit":"C","time":"2025-09-03T10:27:20.724717441Z","type":"ac_temp"}
    Can we prevent this?
  2. Unable to use numeric metadata even when there are other non-numeric metadata fields as follows: 
    {"temp": 22, "meta" : { "unit": "C", "min": 50 }}
    This is just the extension of the same rule that mandates metadata values to be non-numeric. Just wondering if this could be relaxed at least when combined with other non-numeric values in a group.

@albinsuresh
Copy link
Copy Markdown
Contributor

albinsuresh commented Sep 3, 2025

The feature is working fine. Happy to approve once a few queries are clarified. Just noticed a few quirks:

  1. I could publish a measurement with only metadata and no measurement values as follows 
    tedge mqtt pub 'te/device/main///m/ac_temp' '{"unit": "C"}'
    which got converted to 
    [c8y/measurement/measurements/create] {"unit":"C","time":"2025-09-03T10:27:20.724717441Z","type":"ac_temp"}

I can confirm that this isn't a regression but the existing behaviour. The existing logic ignores the "unit" key as well and just publishes: {"type":"C","time":"2025-09-03T11:25:22.521768366Z"}

@didier-wenzek
Copy link
Copy Markdown
Contributor Author

didier-wenzek commented Sep 3, 2025 •
edited
Loading

  1. Unable to use numeric metadata even when there are other non-numeric metadata fields as follows:
    {"temp": 22, "meta" : { "unit": "C", "min": 50 }}
    This is just the extension of the same rule that mandates metadata values to be non-numeric. Just wondering if this could be relaxed at least when combined with other non-numeric values in a group.

This has been discussed here: #3768 (comment).

This could be done, but at the cost of a whole refactoring because the current implementation transforms the message on the fly translating the group as metadata as soon as a text value is seen, or as measurements when the first value is numeric.

Hence, this compromise: #3768 (comment)

Not ideal, I agree.

@didier-wenzek didier-wenzek mentioned this pull request Sep 3, 2025
Closed
@didier-wenzek
Copy link
Copy Markdown
Contributor Author

didier-wenzek commented Sep 4, 2025

Marking this PR as draft, waiting for approval/reject of #3768

@didier-wenzek didier-wenzek marked this pull request as draft September 4, 2025 07:50
@didier-wenzek
Reproduce issue mixing measurements with metadata
9d365bf 
thin-edge#3768

Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
Allow metadata properties along measurements
614daa6 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek
Document non-numeric properties for measurements
bb9c972 
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
@didier-wenzek didier-wenzek force-pushed the fix/non-numeric-measurement-properties branch from b18c973 to bb9c972 Compare September 9, 2025 14:42
@didier-wenzek didier-wenzek changed the title fix: allow non-numeric meta properties in tedge measurements feat: publish properties along tedge measurements Sep 9, 2025
@didier-wenzek didier-wenzek marked this pull request as ready for review September 9, 2025 14:47
reubenmiller
reubenmiller approved these changes Sep 10, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Docs and system tests look good.

rina23q
rina23q approved these changes Sep 10, 2025
View reviewed changes
Copy link
Copy Markdown
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.

LGTM

@didier-wenzek didier-wenzek added this pull request to the merge queue Sep 10, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 10, 2025
@didier-wenzek didier-wenzek added this pull request to the merge queue Sep 10, 2025
Merged via the queue into thin-edge:main with commit db42b3f Sep 10, 2025
34 checks passed
@didier-wenzek didier-wenzek deleted the fix/non-numeric-measurement-properties branch September 11, 2025 07:23
@albinsuresh albinsuresh mentioned this pull request Sep 16, 2025
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@albinsuresh albinsuresh albinsuresh left review comments

@reubenmiller reubenmiller reubenmiller approved these changes

@rina23q rina23q rina23q approved these changes

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

Assignees

No one assigned

Labels

theme:telemetry Theme: Telemetry data

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@didier-wenzek @albinsuresh @reubenmiller @rina23q