Skip to content

Conversation

@aadityaraj7769
Copy link

Summary

This PR adds OpenTelemetry (OTel) metrics instrumentation to the ClusterInfoSensor.

Changes

  • New Interface: Added ClusterInfoOtelMetricsProvider interface for collecting ClusterInfo metrics via OpenTelemetry

    • CanaryDistributionPolicy
  • No-op Implementation: Added NoOpClusterInfoOtelMetricsProvider as default implementation when metrics are disabled

  • Integration:

    • Integrated metrics provider into JmxManager constructor which is called from container MP.
    • Metrics provider passed to ClusterInfoJmx for metrics collection.
  • Metrics Tracked:

    • Gauges: CanaryDistributionPolicy

Testing

  • Updated test fixtures to mock the new metrics provider

Backward Compatibility

  • Fully backward compatible - defaults to no-op provider when not configured

Comment on lines +52 to +59
case STABLE:
_clusterInfoOtelMetricsProvider.recordCanaryDistributionPolicy(clusterName, 0);
return 0;
case CANARY:
_clusterInfoOtelMetricsProvider.recordCanaryDistributionPolicy(clusterName, 1);
return 1;
default:
_clusterInfoOtelMetricsProvider.recordCanaryDistributionPolicy(clusterName, -1);
Copy link

Choose a reason for hiding this comment

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

This seems wrong. Why would a getter function try to record metrics? getter can't have a side effect of "doing" something.

Also, I'm wondering, why rest.li needs to be aware of otel vs rrd? all rest.li layer should do is "expose" metrics to be recorded & helper methods to read those. container code should use those methods & accordingly record in otel or not based on config flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants