Refactoring: Unify and Consolidate Metric CRDs and Handler Logic

[HannesDyballa]

Description

Whilst reviewing the codebase, working on #134, i noticed significant inconsistencies across our four metric CRDs (Metric, ManagedMetric, FederatedMetric, FederatedManagedMetric) and their corresponding handlers.

The current implementations are highly fragmented, with each metric type following its own distinct pattern for API definition and data processing. This has led to substantial code duplication, inconsistent user experience, and increased maintenance complexity.

This issue proposes a refactoring to unify these components, adhere to the DRY principle, and create a more robust and maintainable architecture.

Analysis of Inconsistencies

Analysis not complete, other inconsistencies may come up during refactoring.

We can group the observed problems into two main categories:
1. Inconsistent API Specs (api/v1alpha1/..._types.go)

The Spec for each of the four metric CRDs has diverged significantly.

• Projection/Dimension Fields:

  • Metric and FederatedMetric use Projections []Projection.
  • ManagedMetric in PR Add dimensions managedmetrics #106 is adding Dimensions map[string]string, the PR Add support for map and slice dimensions for metrics and managedmetrics #138 (work in progress) is changing this to Dimensions []Projection. The types ([]struct vs. map) are inconsistent. The renaimng to dimensions is already tracked by Dimensions vs. Projections #135.
  • FederatedManagedMetric has no field for user-defined dimensions/projections at all, although preperatoins where made

    metrics-operator/api/v1alpha1/federatedmanagedmetric_types.go

    Line 38 in d1099b2

    // Projections []Projection `json:"projections,omitempty"`

    .

• Target Resource Definition:

  • ManagedMetric has an optional Target *GroupVersionKind.
  • FederatedManagedMetric has no Target field.

• Status and Observation Structs:

  • The Status objects (MetricStatus, ManagedMetricStatus, etc.) and their nested Observation structs are all unique, making it difficult to create generic status-handling logic.

2. Divergent and Duplicated Handler Logic (internal/orchestrator/...handler.go)

The implementation logic within each handler shows a complete lack of shared code for common tasks.

• Projection/Dimension Processing and Monitoring functions:
  • MetricHandler: Correctly supports multiple projections by grouping resources based on the unique combination of all projected values.
  • ManagedHandler: Implements its own logic for iterating through projections, which is different from MetricHandler.
  • FederatedHandler: Contains a critical limitation, explicitly stating in a comment that it only processes the first projection. Similar to feature: Evaluate multiple projections #104, wich got fixed by Metric multi dimensions #107.

    metrics-operator/internal/orchestrator/federatedhandler.go

    Line 77 in d1099b2

    var dimensions []v1alpha1.Dimension

  • FederatedManagedHandler: Has zero support for processing user-defined projections/dimensions. It only works with status conditions.

Why This Is a Critical Problem

• Violates DRY Principle: We are maintaining four separate implementations for data extraction/ monitoring.
• High Maintenance Overhead: A bug fix or feature enhancement (e.g., improving JSONPath support Add support for map/slice dimensions for Metrics and ManagedMetrics #134) must be implemented and tested in up to four different places.
• Inconsistent User Experience: Users must learn different schemas and expect different behaviors depending on the metric CRD they use. For example, a user might be surprised that FederatedMetric only considers their first projection.
• Technical Debt: The codebase is becoming increasingly complex and difficult to reason about, hindering future development.

Proposed Solution: A Unified Architecture

We should refactor towards a more composable and centralized architecture.

1. Unify the API Specs:

   • Standardize on a single field for defining dimensions across all four metric types. A good candidate is Dimensions []Projection, but renaming the struct to Dimension, aligns with Dimensions vs. Projections #135.
   • Where possible, unify other common fields.

2. Create Shared Utility Packages/Functions:

   • Dimension Extractor: Create a single, well-tested function that takes an unstructured.Unstructured object and a slice of dimension definitions ([]Projection). It should return a map[string]string of the extracted dimension values. This will be used by all current handlers, as this functionality should be shared.

3. Refactor the Handlers:

   • Remove all duplicated logic from the four handlers.
   • Each handler's Monitor function should become much simpler:
     1. For each object, call the DimensionExtractor utility to get its dimensions.
     2. Construct the DataPoint and record the metric. (maybe we could extract this too)

Acceptance Criteria

• The Spec definitions for all four metric types are consistent, particularly for defining dimensions.
• centralize logic for extracting Dimensions and adding tests.
• All four handlers (MetricHandler, ManagedHandler, FederatedHandler, FederatedManagedHandler) are refactored to use the shared utilities.
• Code duplication in the handlers is eliminated.
• All existing functionality is preserved, and a user can now define multiple dimensions on any metric type with consistent behavior.
• The limitation in FederatedHandler (only using the first projection) is removed.

Related: The PRs #106 and #134 are currently editing these files, adding functionality.

[maximiliantech]

I love your proposal. BIG THANKS @HannesDyballa 🚀
The unified architecture with the three refactoring parts look independent from each other, right?. I think it would make sense to split these tasks up into separate tasks/issues with separate PRs so the mental load is not too high.
What do you think?

[HannesDyballa]

I think splitting the issue into smaller more digestible ones is a good idea.
I think 2. Create Shared Utility Packages/Functions can be prepared in isolation to the other issue.

But I would advocate against changing the API and refactor the handlers separately. They are connected rather closely, because the handler implementation uses the spec right?
Also the user would be faced with an updated and consistent API for the metrics, while the implementation is still inconsistent.

Also I just saw #93, which could also be integrated into this refactoring "story"/ "path". Without looking into the issue further, I think this one can be implemented seperately. 👍

[jpraychev]

@HannesDyballa Overall I think the whole API needs a touch up. I lack the knowledge from the past and why things are the way there are right now but here are some comments on the topic that I've been thinking for a while.

• Indeed logic is duplicated which might have been caused by the fact that each metric type has been worked on in difference timelines. I think that metric types were created to handle native resources of k8s, while managedmetric for anything custom like crossplane but underneath these two metrics share the same principle.
  On this topic I would go even one step further and propose that only one resource could be left, a generic Metric, which users use to define w/e metric they would like to export to the backend. The GVK is what should be determining the resource type/version.

  • Scrape defined resources from the cluster - either native or CRs does not matter
  • Construct the OTLP metrics - aggregate values when possible to avoid multiple data points and redundancy
  • Export the metrics

• The same goes for federated metrics and federated managed metrics i guess, even tough I haven't spent enough time so I would restrain myself from making bold comments.

Other topics that are not particularly related to the handler logic but need refactoring imho are:

• Allow users to define values for their metrics in certain cases, e.g. when a job has finished instead of recording that in dimensions
• Aggregation when possible to avoid unnecessary calls
• Clean up dynatrace sink implementation and make it more generic (maybe support major backends and/or otlp collector)

[HannesDyballa]

After the last community call we decided on aligning the implementation of the different metric types. I documented this in #181.
After the implementation is aligned, we can start to align the APIs.

After we have both the implementation and APIs alignt we have a clearer picture on where the differences between the Metrics lie and we can decide if we want to merge some of the APIs or to keep them separate.
This depends on the resulting complexity of a unified API and how complex it might be to maintain the different APIs.