Skip to content

Commit

Permalink
Show metrics needing measurement in the UI.
Browse files Browse the repository at this point in the history
In the UI, show a metric as needing a measurement because the source parameters were recently changed.

Implemented by:
- In the collector: adding a hash of the source parameters to measurements.
- In the API-server: compare the hash of the latest measurement with the hash of the current configuration when adding recent measurements to a report and mark the metric as outdated.
- In the UI: show metrics as needing a measurement if the last measurement is outdated.

Open issues:
- Seeing the source parameter hash is only useful in the most recent measurement, should the hash be removed from the latest measurement before inserting a new one?

Closes #3134.
  • Loading branch information
fniessink committed May 13, 2024
1 parent 05664ed commit 67c0f0f
Show file tree
Hide file tree
Showing 8 changed files with 142 additions and 72 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ async def collect(self) -> MetricMeasurement | None:
source_measurement.source_uuid = source_uuid
if issue_status_collectors:
issue_statuses = await asyncio.gather(*issue_status_collectors)
return MetricMeasurement(measurements, issue_statuses)
return MetricMeasurement(self._metric, measurements, issue_statuses)

def __source_collectors(self) -> list[Coroutine]:
"""Create the source collectors for the metric."""
Expand Down
5 changes: 5 additions & 0 deletions components/collector/src/model/measurement.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

from collections.abc import Sequence

from shared.model.metric import Metric

from collector_utilities.type import URL, ErrorMessage, Value

from .entity import Entities
Expand Down Expand Up @@ -54,9 +56,11 @@ class MetricMeasurement:

def __init__(
self,
metric: Metric,
source_measurements: Sequence[SourceMeasurement],
issue_statuses: Sequence[IssueStatus],
) -> None:
self.metric = metric
self.sources = source_measurements
self.issue_statuses = issue_statuses
self.has_error = any(source_measurement.has_error() for source_measurement in source_measurements)
Expand All @@ -70,6 +74,7 @@ def as_dict(self) -> dict:
"has_error": self.has_error,
"metric_uuid": self.metric_uuid,
"report_uuid": self.report_uuid,
"source_parameter_hash": self.metric.source_parameter_hash()
}
if self.issue_statuses:
measurement["issue_status"] = [issue_status.as_dict() for issue_status in self.issue_statuses]
Expand Down
4 changes: 4 additions & 0 deletions components/collector/tests/base_collectors/test_collector.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ def expected_source(self, **kwargs: str | None) -> dict[str, str | None | list[d
def expected_measurement(
self,
*,
source_parameter_hash: str = "0f6df164677525409f6269ec97e4118d",
metric_uuid: str = "metric_uuid",
**expected_source_kwargs,
) -> dict[str, bool | list | str]:
Expand All @@ -103,6 +104,7 @@ def expected_measurement(
"sources": [self.expected_source(**expected_source_kwargs)],
"metric_uuid": metric_uuid,
"report_uuid": "report1",
"source_parameter_hash": source_parameter_hash,
}

async def test_fetch_successful(self):
Expand Down Expand Up @@ -228,6 +230,7 @@ async def test_prioritize_edited_metrics(self):
expected_call2 = call(
self.database,
self.expected_measurement(
source_parameter_hash="ebfcffc74dc017799cb4da33090a9867",
metric_uuid="metric_uuid2",
api_url=edited_url,
landing_url=edited_url,
Expand Down Expand Up @@ -261,6 +264,7 @@ async def test_missing_mandatory_parameter_with_default_value(self):
post.assert_called_once_with(
self.database,
self.expected_measurement(
source_parameter_hash="8c3b464958e9ad0f20fb2e3b74c80519",
value="0",
entities=[],
api_url="",
Expand Down
22 changes: 20 additions & 2 deletions components/frontend/src/measurement/MeasurementValue.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useContext } from "react"
import { Message } from "semantic-ui-react"
import { Icon, Message } from "semantic-ui-react"

import { DataModel } from "../context/DataModel"
import { Label, Popup } from "../semantic_ui_react_wrappers"
Expand All @@ -18,7 +18,18 @@ export function MeasurementValue({ metric, reportDate }) {
const end = new Date(metric.latest_measurement.end)
const now = reportDate ?? new Date()
const staleMeasurementValue = now - end > MILLISECONDS_PER_HOUR // No new measurement for more than one hour means something is wrong
const trigger = staleMeasurementValue ? <Label color="red">{value}</Label> : <span>{value}</span>
let trigger
if (staleMeasurementValue) {
trigger = <Label color="red">{value}</Label>
} else if (metric.latest_measurement.outdated) {
trigger = (
<Label color="yellow">
<Icon loading name="hourglass" /> {value}
</Label>
)
} else {
trigger = <span>{value}</span>
}
return (
<Popup trigger={trigger} flowing hoverable>
{staleMeasurementValue && (
Expand All @@ -28,6 +39,13 @@ export function MeasurementValue({ metric, reportDate }) {
content="This may indicate a problem with Quality-time itself. Please contact a system administrator."
/>
)}
{metric.latest_measurement.outdated && (
<Message
warning
header="Latest measurement out of date"
content="The source configuration of this metric was changed after the latest measurement."
/>
)}
<TimeAgoWithDate date={metric.latest_measurement.end}>
{metric.status ? "The metric was last measured" : "Last measurement attempt"}
</TimeAgoWithDate>
Expand Down
25 changes: 25 additions & 0 deletions components/frontend/src/measurement/MeasurementValue.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,31 @@ it("renders a value that has not been measured yet", () => {
expect(screen.getAllByText(/\?/).length).toBe(1)
})

it("renders an outdated value", async () => {
render(
<DataModel.Provider value={{ metrics: { violations: { unit: "violations" } } }}>
<MeasurementValue
metric={{
type: "violations",
scale: "count",
unit: null,
latest_measurement: { count: { value: 1 }, outdated: true },
}}
/>
</DataModel.Provider>,
)
const measurementValue = screen.getByText(/1/)
expect(measurementValue.className).toContain("yellow")
expect(measurementValue.children[0].className).toContain("loading")
await userEvent.hover(measurementValue)
await waitFor(() => {
expect(screen.queryByText(/Latest measurement out of date/)).not.toBe(null)
expect(
screen.queryByText(/The source configuration of this metric was changed after the latest measurement/),
).not.toBe(null)
})
})

it("renders a minutes value", () => {
render(
<DataModel.Provider value={{ metrics: { duration: { unit: "minutes" } } }}>
Expand Down
10 changes: 9 additions & 1 deletion components/shared_code/src/shared/model/measurement.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from abc import abstractmethod
from collections.abc import Sequence
from typing import TYPE_CHECKING, ClassVar, cast
from typing import TYPE_CHECKING, ClassVar, Self, cast

from packaging.version import InvalidVersion, Version

Expand Down Expand Up @@ -294,3 +294,11 @@ def summarize(self) -> dict[str, str | dict[str, str | None]]:
"start": self["start"],
"end": self["end"],
}

def summarize_latest(self) -> Self:
"""Return a summary of this measurement, given that it is the latest measurement."""
if parameter_hash := self.get("source_parameter_hash"):
if parameter_hash != self.metric.source_parameter_hash():
self["outdated"] = True
del self["source_parameter_hash"]
return self
8 changes: 7 additions & 1 deletion components/shared_code/src/shared/model/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from datetime import date
from typing import TYPE_CHECKING, cast

from shared.utils.functions import md5_hash
from shared.utils.type import (
Direction,
MetricId,
Expand Down Expand Up @@ -159,9 +160,14 @@ def summarize(self, measurements: list[Measurement], **kwargs) -> dict:
summary["scale"] = self.scale()
summary["status"] = self.status(latest_measurement)
summary["status_start"] = latest_measurement.status_start() if latest_measurement else None
summary["latest_measurement"] = latest_measurement
summary["latest_measurement"] = latest_measurement.summarize_latest() if latest_measurement else None
summary["recent_measurements"] = [measurement.summarize() for measurement in measurements]
if latest_measurement:
summary["issue_status"] = self.issue_statuses(latest_measurement)
summary.update(kwargs)
return summary

def source_parameter_hash(self) -> str:
"""Return a hash of the source parameters that can be used to check for changed parameters."""
source_parameters = [source.get("parameters") for source in self.sources]
return md5_hash(str(source_parameters))
138 changes: 71 additions & 67 deletions components/shared_code/tests/shared/model/test_metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,91 +25,91 @@ def create_metric(self, **kwargs) -> Metric:
"""Return a metric fixture."""
return Metric(self.DATA_MODEL, {"type": "fixture_metric_type", **kwargs}, METRIC_ID)

def create_expected_summary(
self, status: str = "", measurement_timestamp: str = "", outdated: bool = False, **kwargs
) -> dict[str, None | str | list | dict]:
"""Return an expected summary."""
if measurement_timestamp:
latest_measurement = {
"count": {"value": 1, "start": measurement_timestamp},
"end": measurement_timestamp,
"sources": [],
"start": measurement_timestamp,
"status": status,
}
recent_measurements = [
{
"count": {"status": status, "value": 1},
"end": measurement_timestamp,
"start": measurement_timestamp,
},
]
else:
latest_measurement = None
recent_measurements = []
summary = {
"type": "fixture_metric_type",
"scale": "count",
"status": status if status else None,
"status_start": None,
"latest_measurement": latest_measurement,
"recent_measurements": recent_measurements,
"sources": {},
**kwargs,
}
if measurement_timestamp:
summary["issue_status"] = []
if outdated:
summary["latest_measurement"]["outdated"] = True
return summary

def test_summarize_empty_metric(self):
"""Test that a minimal metric returns a summary."""
summary = self.create_metric().summarize([])
self.assertDictEqual(
summary,
{
"type": "fixture_metric_type",
"scale": "count",
"status": None,
"status_start": None,
"latest_measurement": None,
"recent_measurements": [],
"sources": {},
},
self.assertDictEqual(summary, self.create_expected_summary())

def test_summarize_metric_with_a_measurement(self):
"""Test that the metric summary includes the measurement."""
metric = self.create_metric()
measurement_timestamp = iso_timestamp()
measurement = Measurement(
metric,
count={"value": 1, "start": measurement_timestamp},
status="target_met",
)
summary = metric.summarize([measurement])
self.assertDictEqual(summary, self.create_expected_summary("target_met", measurement_timestamp))

def test_summarize_measurements(self):
"""Test that a minimal metric returns a summary."""
def test_summarize_metric_with_an_outdated_measurement(self):
"""Test that the metric is marked outdated when the source parameters have changed."""
metric = self.create_metric()
measurement_timestamp = iso_timestamp()
measurement = Measurement(
metric,
count={"value": 1, "start": measurement_timestamp},
status="target_met",
source_parameter_hash="old hash",
)
summary = metric.summarize([measurement])
self.assertDictEqual(
summary,
{
"issue_status": [],
"type": "fixture_metric_type",
"scale": "count",
"status": "target_met",
"status_start": None,
"latest_measurement": {
"count": {"value": 1, "start": measurement_timestamp},
"end": measurement_timestamp,
"sources": [],
"start": measurement_timestamp,
"status": "target_met",
},
"recent_measurements": [
{
"count": {"status": "target_met", "value": 1},
"end": measurement_timestamp,
"start": measurement_timestamp,
},
],
"sources": {},
},
self.assertDictEqual(summary, self.create_expected_summary("target_met", measurement_timestamp, outdated=True))

def test_summarize_metric_with_an_up_to_date_measurement(self):
"""Test that the metric is not marked outdated when the source parameters have not changed."""
metric = self.create_metric()
measurement_timestamp = iso_timestamp()
measurement = Measurement(
metric,
count={"value": 1, "start": measurement_timestamp},
status="target_met",
source_parameter_hash=metric.source_parameter_hash(),
)
summary = metric.summarize([measurement])
self.assertDictEqual(summary, self.create_expected_summary("target_met", measurement_timestamp))

def test_summarize_metric_data(self):
"""Test that metric data is summarized."""
summary = self.create_metric(some_metric_property="some_value").summarize([])
self.assertDictEqual(
summary,
{
"type": "fixture_metric_type",
"scale": "count",
"status": None,
"status_start": None,
"latest_measurement": None,
"recent_measurements": [],
"some_metric_property": "some_value",
"sources": {},
},
)

def test_summarize_metric_kwargs(self):
"""Test that metric data is summarized."""
summary = self.create_metric().summarize([], some_kw="some_arg")
self.assertDictEqual(
summary,
{
"type": "fixture_metric_type",
"scale": "count",
"status": None,
"status_start": None,
"latest_measurement": None,
"recent_measurements": [],
"some_kw": "some_arg",
"sources": {},
},
)
self.assertDictEqual(summary, self.create_expected_summary(some_metric_property="some_value"))

def test_debt_end_date(self):
"""Test debt end date."""
Expand Down Expand Up @@ -151,3 +151,7 @@ def test_not_equal(self):
def test_not_equal_to_string(self):
"""Test that a metric is not equal to a string."""
self.assertNotEqual(self.create_metric(), "foo")

def test_source_parameter_hash(self):
"""Test the source parameter hash."""
self.assertEqual("d751713988987e9331980363e24189ce", self.create_metric().source_parameter_hash())

0 comments on commit 67c0f0f

Please sign in to comment.