Tracability improvements - needs type autodiscovery#545
Conversation
FScholPer
requested review from
AlexanderLanin,
MaximilianSoerenPollak,
dcalavrezo-qorix and
nradakovic
as code owners
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
|
Bit confused about this, can you explain what this is suppose to solve / help with? |
|
sure the problem occurred in the score repo after adding the new metrics.json from docs-as-code. There was no config set up https://github.com/eclipse-score/docs-as-code/blob/main/docs/how-to/dashboards_and_quality_gates.rst it produced metrics like tool_req which does not make sense in platform repo. I added autodiscovery for requirement types so that explicit definition is not needed as default |
Okay, still a bit confused though. Would that not be an easier way? Though I might still not 100% understand/grasp it and this is not possible for this. |
|
we want only requirement types AFAIK |
Yes we chated in slack and I got it explained a bit better and understand now what is wanted here. |
Good point, and agreed that app.config.needs_types is the right source after metamodel setup. In metrics generation we intentionally do one extra step: we filter those registered requirement types to the ones actually present in the current repo needs (respecting include_external). If we only take app.config.needs_types as-is, we can still produce irrelevant entries like tool_req in repos that don’t use it. So the implementation is ‘needs_types-based autodiscovery + present-in-build filtering’. |
AlexanderLanin
left a comment
AlexanderLanin
left a comment
There was a problem hiding this comment.
AI-assisted review (Claude):
Critical: 0, Important: 2, Suggestions: 2
The autodiscovery approach is sound — intersecting metamodel-tagged requirement types with types actually present in the build is the right way to avoid irrelevant metrics entries like tool_req in repos that don't use it.
Two structural concerns with the current implementation, plus two minor items.
|
Updated with fixes |
MaximilianSoerenPollak
left a comment
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Some more questions.
| def _get_need_value(need: Any, key: str, default: Any = None) -> Any: | ||
| return need.get(key, default) |
There was a problem hiding this comment.
This abstraction makes no sense.
You use this once. Abstraction just for the sake of abstraction is not good.
| def _as_requirement_directive(need_type: Any) -> str | None: | ||
| if not isinstance(need_type, dict): | ||
| return None | ||
| directive = need_type.get("directive") | ||
| tags = need_type.get("tags", []) | ||
| if not isinstance(directive, str) or not isinstance(tags, list): | ||
| return None | ||
| normalized = {str(tag).strip() for tag in tags} | ||
| if "requirement_excl_process" in normalized or "requirement" in normalized: | ||
| return directive | ||
| return None |
There was a problem hiding this comment.
I don't quiet understand this.
What exactly do you want to get here, all need types that are directives?
That would be all types you get from app.config.need_types.
If you want to get our own ScoreNeedType which defines some additional things which makes checking easier, see signature:
class ScoreNeedType(NeedType):
tags: list[str]
parts: int
mandatory_options: dict[str, str]
optional_options: dict[str, str]
mandatory_links: AllowedLinksType
optional_links: AllowedLinksTypeThen you could just copy the function we have in check_options which goes like:
def get_need_type(needs_types: list[ScoreNeedType], directive: str) -> ScoreNeedType:
for need_type in needs_types:
assert isinstance(need_type, dict), need_type
if need_type["directive"] == directive:
return need_type
raise ValueError(f"Need type {directive} not found in needs_types")And adapt it so you I guess can filter directly for the tag as well so you don't waste itterations:
def get_need_type(needs_types: list[ScoreNeedType], directive: str) -> ScoreNeedType:
for need_type in needs_types:
assert isinstance(need_type, dict), need_type
if need_type["directive"] == directive and ("requirement_excl_process" in need_type["tags"] or "requirement" in need_type["tags"]:
return need_type
raise ValueError(f"Need type {directive} not found in needs_types")| def _discover_requirement_types( | ||
| app: Sphinx, all_needs: list[Any], include_external: bool | ||
| ) -> set[str]: | ||
| """Discover requirement directives that are both tagged and present.""" | ||
| tagged_requirements: set[str] = set() | ||
| needs_types = getattr(app.config, "needs_types", []) | ||
| for need_type in needs_types or []: | ||
| directive = _as_requirement_directive(need_type) | ||
| if directive: | ||
| tagged_requirements.add(directive) |
There was a problem hiding this comment.
app.config.need_types will give you all need types that are possible.
If you then iterate over all needs you will only get present needs. Sphinx-needs will not give you empty needs that aren''t defined in the project.
So in my opinion this filter makes not much sense.
You could get all needs like this:
all_local_needs = SphinxNeedsData(app.env).get_needs_view().filter_is_external(False)
This already filters the needs and exludes all external ones.
|
Thanks for the comments will continue next week... |
AlexanderLanin
left a comment
AlexanderLanin
left a comment
There was a problem hiding this comment.
AI-assisted review (Claude):
Critical: 0, Important: 2, Suggestions: 1
This pass covers score_metrics (the new extension), the CI workflow rewrite, and cross-cutting consistency. The autodiscovery design in score_metamodel is already well-covered by the existing review comments.
Core issue — score_metamodel_yaml coupling is unguarded. score_metrics/traceability_metrics.py:131 does app.config.score_metamodel_yaml, but that config value is registered by score_metamodel.setup(), not score_metrics. In the bundle (score_sphinx_bundle) score_metamodel is loaded first, so it happens to work — but score_metrics is documented as a standalone extension and its setup() does not declare this dependency. A consumer loading only score_metrics will hit AttributeError. At minimum, score_metrics should either register score_metamodel_yaml itself (with the same default "") or document the hard dependency on score_metamodel.
Tag mismatch — inline comment on traceability_metrics.py:141. The config description says autodiscovery uses {"requirement", "requirement_excl_process"}, but the code passes only {"requirement"}. Types tagged exclusively with requirement_excl_process are silently excluded. Doc and code need to agree on which tags drive discovery.
AI-generated docs — inline comment on dashboards_and_quality_gates.rst:156. ~295 lines of Copilot-generated API reference content were committed into a user-facing how-to guide. It documents internal functions and implementation details. Should be removed.
Minor: stale module path in docstring — inline comment on sphinx_filters.py. The :filter-func: example still says score_metamodel.sphinx_filters.*, which no longer exists.
No concern with the CI workflow rewrite — the simplification is well-motivated and the parametrized test matrix is cleaner. The removal of --lockfile_mode=error from bazel run //:ide_support is worth a quick confirmation that it was intentional.
| print(f"Traceability metrics written to: {out_path}") | ||
|
|
||
|
|
||
| def setup(app: Sphinx) -> dict[str, str | bool]: |
There was a problem hiding this comment.
AI-assisted review (Claude):
Important — score_metamodel_requirement_types is registered by score_metrics, not score_metamodel
score_metrics/__init__.py registers score_metamodel_requirement_types and score_metamodel_include_external_needs as config values, then calls app.setup_extension("score_metamodel"). This means:
- The config values are named
score_metamodel_*but owned byscore_metrics— confusing and semantically wrong. - If
score_metamodelis loaded beforescore_metrics(e.g. standalone, without the bundle),score_metamodel_yamlis registered but these two keys are not, making the pairing asymmetric. - Any consumer who loads both extensions independently could hit a duplicate
add_config_valueregistration error.
These config keys should either be renamed to score_metrics_* to reflect actual ownership, or score_metamodel should register them and score_metrics should just read them.
| Default Behavior (No Configuration Needed) | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| By default, ``score_metamodel`` autodiscovers requirement types from the |
There was a problem hiding this comment.
AI-assisted review (Claude):
Important — docs still say score_metamodel writes metrics.json
Lines 49, 77 (and surrounding context) say:
"By default,
score_metamodelautodiscovers requirement types..."
"The documentation build writesmetrics.jsonviascore_metamodel..."
But after this PR, it is score_metrics that writes metrics.json. score_metamodel does not have this role anymore. Any consumer following these docs will be confused about which extension controls the metric output.
| """ | ||
| parser = argparse.ArgumentParser( | ||
| description=( | ||
| "Read a traceability metrics JSON (schema v1) and enforce coverage " |
There was a problem hiding this comment.
AI-assisted review (Claude):
Suggestion — argparse description says "schema v1" but gate enforces schema v2
description=(
"Read a traceability metrics JSON (schema v1) and enforce coverage "
..._SUPPORTED_SCHEMA_VERSION = "2" — this will appear verbatim in --help output and confuse users running the gate.
| "Read a traceability metrics JSON (schema v1) and enforce coverage " | |
| description=( | |
| "Read a traceability metrics JSON (schema v2) and enforce coverage " | |
| ... |
AlexanderLanin
left a comment
AlexanderLanin
left a comment
There was a problem hiding this comment.
AI-assisted review (Claude):
Re-review pass — Critical: 0, Important: 3, Suggestions: 1
Comment resolution status
✅ Fixed since last review:
_reqsuffix fallback — removed_get_need_value/except Exceptionover-defensiveness — removeddocs/conf.pyexplicit config noise — removed_as_requirement_directivepointless abstraction — removed- Stale
score_metamodel.sphinx_filtersmodule path in docstring — fixed
❌ Still open:
- Tag mismatch (
{"requirement"}vs documented{"requirement", "requirement_excl_process"}) — not fixed - AI-generated Copilot block in
dashboards_and_quality_gates.rst— still present at line ~155 score_metamodel_yamlimplicit dependency —score_metricsreadsapp.config.score_metamodel_yamlbut does not register it; relies onapp.setup_extension("score_metamodel")being called first
Architecture summary
The split is the right direction: score_metamodel → metamodel/validation, score_metrics → metric collection + CI gate output. The deletion of the old traceability_metrics.py from score_metamodel is clean. No live duplication.
Three structural issues with the current state:
1. Config key ownership mismatch (inline comment on score_metrics/__init__.py:53).
score_metamodel_requirement_types and score_metamodel_include_external_needs are registered by score_metrics, not score_metamodel. Names imply one owner, code says another. Risk of duplicate registration if both extensions are loaded independently.
2. Docs attribute metrics.json to the wrong extension (inline comment on dashboards_and_quality_gates.rst:49).
Multiple lines say score_metamodel writes metrics.json. It does not — score_metrics does. Anyone configuring or troubleshooting metrics generation will look in the wrong place.
3. Schema v2 is a silent breaking change for existing consumers.
The gate now rejects schema v1 with a hard error. There is no mention of this in the docs, no migration guide, and no CHANGELOG entry. Any downstream consumer running the old gate against a new build (or vice versa) will get unsupported schema_version 1 with no upgrade path. This should at minimum be called out in the how-to guide.
4. Argparse description says "schema v1" (inline comment on traceability_gate.py:200).
One-line fix, shows up in --help.
Gate correctness
The gate logic itself is sound for the common case. One edge case worth noting: safe_percent(0, 0) returns 100.0, so a build with zero testcases will report linked_to_requirements_pct = 100.0 and pass any --min-tests-linked threshold. The gate does guard the empty metrics_by_type case correctly, but not the zero-tests case. Whether this is acceptable behavior is a product decision — if intentional, it should be documented.
4 participants
📌 Description
Improved metrics.json by
🚨 Impact Analysis
✅ Checklist
Frank Scholter Peres frank.scholter_peres@mercedes-benz.com, Mercedes-Benz Tech Innovation GmbH
Provider Information