chore: refactor metrics classes [DO NOT MERGE WITHOUT CHANGING BASE TO MAIN]

[yanksyoon]

Applicable spec:

Overview

• Make RunnerMetrics into an interface READONLY dataclass using Protocol + @Property
• Refactor complex _pull_runner_metrics function
• Merge parsing + instantiating in PulledMetrics class
• Rename runner_installed & other data attributes to be more descriptive

Rationale

• This allows capturing accurate data for Prometheus metrics and disallows mutating data

Juju Events Changes

Module Changes

Library Changes

Checklist

• The charm style guide was applied.
• The contributing guide was applied.
• The changes are compliant with ISD054 - Managing Charm Complexity
• The documentation for charmhub is updated.
• The PR is tagged with appropriate label (urgent, trivial, complex).
• The changelog is updated with changes that affects the users of the charm.
• The application version number is updated in github-runner-manager/pyproject.toml.

No user facing changes

[github-actions]

Test results for commit 4a25772

Test coverage for 4a25772

Wrote XML report to coverage/coverage.xml

Static code analysis report

Run started:2025-08-06 08:57:59.773666

Test results:
  No issues identified.

Code scanned:
  Total lines of code: 2026
  Total lines skipped (#nosec): 2
  Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 1

Run metrics:
  Total issues (by severity):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
  Total issues (by confidence):
  	Undefined: 0
  	Low: 0
  	Medium: 0
  	High: 0
Files skipped (0):

[cbartz]

Thanks! Some questions/remarks about semantic changes, otherwise it looks good.

[cbartz]

Before it was

    installation_start_timestamp: NonNegativeFloat | None
    installed_timestamp: NonNegativeFloat

so it seems it should be

Suggested change

	def installation_start_timestamp(self) -> NonNegativeFloat: # type: ignore
	"""UNIX timestamp of in which the VM setup started."""
	@property
	def installation_end_timestamp(self) -> NonNegativeFloat | None:
	def installation_start_timestamp(self) -> NonNegativeFloat | None: # type: ignore
	"""UNIX timestamp of in which the VM setup started."""
	@property
	def installation_end_timestamp(self) -> NonNegativeFloat:

[yanksyoon]

I think the previous implementation as wrong (diff here).

Installation start timestamp is referring to the OpenStack VM created_at timestamp which the type hint suggests that it cannot be None.
Installation end timestamp, however, is a metric that is pulled over SSH connection which could be None due to some failures.

One question I have is whether Installation start timestamp (created_at timestamp) from OpenStack VM might be None which I don't think it shoud?

[cbartz]

Suggested change

	def installation_end_timestamp(self) -> NonNegativeFloat | None:
	def installation_end_timestamp(self) -> NonNegativeFloat:

If I am correct with my other comment above

[cbartz]

Suggested change

	installation_end_timestamp = runner_metrics.installation_end_timestamp or 0
	installation_end_timestamp = runner_metrics.installation_end_timestamp

if I am correct with my other comment above

[cbartz]

why the max change here, which looks like a semantic change?

[cbartz]

Suggested change

	if pre_job_metrics.timestamp < (runner_metrics.installation_end_timestamp or 0):
	if pre_job_metrics.timestamp < runner_metrics.installation_end_timestamp:

if I am correct with my comment above that the timestamp should always be present

[cbartz]

This seems to be a semantic change: Before the PR , if the timestamp was not present, we would not issue any metrics at all

        if self.runner_installed is None:
            logger.error(
                "Invalid pulled metrics. No runner_installed information for %s.", instance_id
            )
            return None

[yanksyoon]

Leaving a comment after discussion:

This semantics change is proposed to convey metrics for instances that may have failed to initialize.
Conveying None values for all metrics would suggest that some step has gone wrong and let us decide to perhaps put it in +inf bucket.
This metric would show us how many times we were failing to either instantiate or SSH into the machine to fetch the metrics, helping us decide the next move.

[cbartz]

This test seems gone, are these cases covered in the new tests?

[yanksyoon]

Yup, they have been better parametrized by the "state" input, rather than testing for the internal function calls.
The parametrized tests are:

• no instances
• single instance, no metrics
• single instance, partial metrics(POST_JOB_METRICS_FILE_PATH): post job only
• single instance, partial metrics(PRE_JOB_METRICS_FILE_PATH): pre job only
• single instance, partial metrics(RUNNER_INSTALLED_TS_FILE_PATH): runner installed timestamp only
• single instance, all metrics
• multi instance, all metrics

These cases more than cover the existing tests, but go beyond and extend the combinatorics.