Skip to content

Conversation

@flora-hofmann-frequenz
Copy link
Collaborator

Example:MetricSample(timestamp=datetime.datetime(2025, 7, 8, 9, 10, 20, tzinfo=datetime.timezone.utc), microgrid_id=13, component_id=262, metric='BATTERY_SOC_PCT_avg', value=43.198001861572266),
MetricSample(timestamp=datetime.datetime(2025, 7, 8, 9, 10, 20, tzinfo=datetime.timezone.utc), microgrid_id=13, component_id=262, metric='BATTERY_SOC_PCT_min', value=42.70000076293945),
MetricSample(timestamp=datetime.datetime(2025, 7, 8, 9, 10, 20, tzinfo=datetime.timezone.utc), microgrid_id=13, component_id=262, metric='BATTERY_SOC_PCT_max', value=43.29999923706055),

@github-actions github-actions bot added the part:docs Affects the documentation label Jul 9, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Updates the metric sample iterator to also expose aggregated metrics and documents this new capability.

  • Adds handling for aggregated_metric (avg, min, max, raw values) in __iter__.
  • Removes the old math.nan fallback for missing simple metrics.
  • Updates release notes to mention the new feature.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/frequenz/client/reporting/_types.py Expanded __iter__ to support aggregated_metric variants and dropped the NaN fallback.
RELEASE_NOTES.md Added a bullet noting that aggregated metrics are now exposed.
Comments suppressed due to low confidence (3)

src/frequenz/client/reporting/_types.py:104

  • Add unit tests covering the new aggregated metric logic (avg, min, max, raw values) to verify correct iteration behavior and catch future regressions.
                if sample.value.HasField("aggregated_metric"):

RELEASE_NOTES.md:13

  • [nitpick] For consistency with the surrounding list style, replace the leading * with - in this bullet point.
* Expose aggregated metrics if available.

src/frequenz/client/reporting/_types.py:104

  • [nitpick] Use elif instead of a second if when checking for aggregated_metric to ensure exclusive handling and avoid evaluating both branches if both fields were ever present.
                if sample.value.HasField("aggregated_metric"):

@flora-hofmann-frequenz flora-hofmann-frequenz added this pull request to the merge queue Jul 9, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 73cbce9 Jul 9, 2025
5 checks passed
@flora-hofmann-frequenz flora-hofmann-frequenz deleted the add_agg_metrics branch July 9, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants