Skip to content

Conversation

@britaniar
Copy link
Contributor

@britaniar britaniar commented May 15, 2025

What type of PR is this?
/kind bug

What this PR does / why we need it:
Change the metrics registry as we use controller runtime to emit metrics. Previously, with using the prometheus package nothing was being emitted therefore not scraped.

Which issue(s) this PR fixes:

Fixes #

Requirements:

How has this code been tested

Special notes for your reviewer

@kaito-pr-agent
Copy link

kaito-pr-agent bot commented May 15, 2025

Title

(Describe updated until commit 9383736)

Update metrics registration to use controller runtime registry


Description

  • Updated metrics registration to use controller runtime registry

  • Removed custom Prometheus registry initialization and unregistration

  • Updated tests to use controller runtime metrics registry


Changes walkthrough 📝

Relevant files
Bug fix
controller.go
Update metrics registration                                                           

pkg/controllers/hub/trafficmanagerbackend/controller.go

  • Imported ctrlmetrics package
  • Updated metrics registration to use ctrlmetrics.Registry
  • +4/-2     
    controller.go
    Update metrics registration                                                           

    pkg/controllers/hub/trafficmanagerprofile/controller.go

  • Imported ctrlmetrics package
  • Updated metrics registration to use ctrlmetrics.Registry
  • +4/-2     
    Tests
    controller_integration_test.go
    Update integration tests for metrics                                         

    pkg/controllers/hub/trafficmanagerbackend/controller_integration_test.go

  • Removed custom Prometheus registry
  • Updated metric validation to use ctrlmetrics.Registry
  • Renamed initialization function to reset function
  • +59/-113
    controller_integration_test.go
    Update integration tests for metrics                                         

    pkg/controllers/hub/trafficmanagerprofile/controller_integration_test.go

  • Removed custom Prometheus registry
  • Updated metric validation to use ctrlmetrics.Registry
  • Renamed initialization function to reset function
  • +37/-88 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented May 15, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 9383736)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Metric Validation

    Ensure that the metrics validation logic correctly handles the new metrics registry and that all test cases cover the expected metrics.

    Eventually(func() error {
    	metricFamilies, err := ctrlmetrics.Registry.Gather()
    	if err != nil {
    		return fmt.Errorf("failed to gather metrics: %w", err)
    	}
    	var gotMetrics []*prometheusclientmodel.Metric
    	for _, mf := range metricFamilies {
    Metric Validation

    Ensure that the metrics validation logic correctly handles the new metrics registry and that all test cases cover the expected metrics.

    var _ = Describe("Test TrafficManagerProfile Controller", func() {
    	Context("When updating existing valid trafficManagerProfile", Ordered, func() {
    		name := fakeprovider.ValidProfileName
    		var profile *fleetnetv1beta1.TrafficManagerProfile
    		profileResourceID := fmt.Sprintf(fakeprovider.ProfileResourceIDFormat, fakeprovider.DefaultSubscriptionID, fakeprovider.DefaultResourceGroupName, name)
    
    		relativeDNSName := fmt.Sprintf(DNSRelativeNameFormat, testNamespace, name)

    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented May 15, 2025

    PR Code Suggestions ✨

    Latest suggestions up to d72404a

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Clean up comment formatting

    Remove unnecessary comment slashes.

    pkg/controllers/hub/trafficmanagerbackend/controller.go [49-51]

    -/// Register trafficManagerBackendStatusLastTimestampSeconds (fleet_networking_traffic_manager_backend_status_last_timestamp_seconds)
    +// Register trafficManagerBackendStatusLastTimestampSeconds (fleet_networking_traffic_manager_backend_status_last_timestamp_seconds)
     // metric with the controller runtime global metrics registry.
     ctrlmetrics.Registry.MustRegister(trafficManagerBackendStatusLastTimestampSeconds)
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion removes unnecessary comment slashes, which improves code readability slightly but offers only a minor improvement.

    Low

    Previous suggestions

    Suggestions up to commit 26d00c7
    Suggestions up to commit 26d00c7
    CategorySuggestion                                                                                                                                    Impact
    General
    Clean up comments

    Remove unnecessary comment lines.

    pkg/controllers/hub/trafficmanagerbackend/controller.go [49-51]

    -/// Register trafficManagerBackendStatusLastTimestampSeconds (fleet_networking_traffic_manager_backend_status_last_timestamp_seconds)
    -// metric with the controller runtime global metrics registry.
     ctrlmetrics.MustRegister(trafficManagerBackendStatusLastTimestampSeconds)
    Suggestion importance[1-10]: 3

    __

    Why: Removing unnecessary comment lines improves code readability slightly but offers minimal functional improvement.

    Low

    Copy link
    Contributor

    @jwtty jwtty left a comment

    Choose a reason for hiding this comment

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

    Do we not need any change in test?

    @codecov
    Copy link

    codecov bot commented May 15, 2025

    Codecov Report

    All modified and coverable lines are covered by tests ✅

    Project coverage is 80.32%. Comparing base (f460412) to head (9383736).
    Report is 1 commits behind head on main.

    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #299      +/-   ##
    ==========================================
    + Coverage   80.28%   80.32%   +0.03%     
    ==========================================
      Files          29       29              
      Lines        4099     4101       +2     
    ==========================================
    + Hits         3291     3294       +3     
    + Misses        643      642       -1     
      Partials      165      165              

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    🚀 New features to boost your workflow:
    • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

    @jwtty
    Copy link
    Contributor

    jwtty commented May 16, 2025

    LGTM

    @jwtty jwtty requested a review from zhiying-lin May 16, 2025 00:57
    Copy link
    Contributor

    @zhiying-lin zhiying-lin left a comment

    Choose a reason for hiding this comment

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

    a minor comment. Others LGTM

    @zhiying-lin zhiying-lin merged commit 0076b8d into Azure:main May 16, 2025
    14 checks passed
    @britaniar britaniar deleted the fixMetrics branch May 19, 2025 17:00
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants