Skip to content

Conversation

@zhiying-lin
Copy link
Contributor

@zhiying-lin zhiying-lin commented Apr 15, 2025

What type of PR is this?

/kind feature

add status metrics,

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

How has this code been tested

added integration tests
e2e tests are green, https://github.com/Azure/fleet-networking/actions/runs/14467440679

Special notes for your reviewer

@kaito-pr-agent
Copy link

kaito-pr-agent bot commented Apr 15, 2025

Title

(Describe updated until commit 626bb4f)

Add status metrics for TrafficManagerProfile


Description

  • Added status metrics for TrafficManagerProfile to monitor the last update timestamp.

  • Registered a custom Prometheus metric for tracking the status of TrafficManagerProfile.

  • Updated the controller to manage the new MetricsFinalizer.

  • Enhanced the integration tests to validate the emission of the new metrics.


Changes walkthrough 📝

Relevant files
Enhancement
objectmeta.go
Add MetricsFinalizer and initialize Prometheus metrics     

pkg/common/objectmeta/objectmeta.go

  • Added MetricsFinalizer constant.
  • Initialized custom Prometheus metrics.
  • +3/-0     
    controller.go
    Manage MetricsFinalizer and emit status metrics                   

    pkg/controllers/hub/trafficmanagerprofile/controller.go

  • Registered the custom Prometheus metrics.
  • Managed the MetricsFinalizer in the reconciliation loop.
  • Emitted the traffic manager profile status metric.
  • +69/-13 
    controller_integration_test.go
    Initialize custom registry and validate metrics emission 

    pkg/controllers/hub/trafficmanagerprofile/controller_integration_test.go

  • Initialized a custom Prometheus registry for testing.
  • Validated the emission of the traffic manager profile status metrics.
  • +213/-9 
    metrics.go
    Create metrics utility package                                                     

    test/common/metrics/metrics.go

    • Created a utility package for testing Prometheus metrics.
    +29/-0   
    Refactoring
    profile.go
    Sort slices in comparison options                                               

    test/common/trafficmanager/validator/profile.go

    • Sorted slices in comparison options for better test stability.
    +3/-0     

    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 Apr 15, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 626bb4f)

    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

    Potential Race Condition

    The emitTrafficManagerProfileStatusMetric function is called within a deferred function, but it doesn't guarantee that the metric will be emitted before the function returns. This could lead to race conditions where the metric is not yet emitted when the function exits.

    // TODO: replace the following with defaulter wehbook
    Missing Error Handling

    The emitTrafficManagerProfileStatusMetric function calls meta.FindStatusCondition, but it doesn't check if the condition is nil before accessing its fields. This could cause a panic if the condition is nil.

    cond := meta.FindStatusCondition(profile.Status.Conditions, string(fleetnetv1beta1.TrafficManagerProfileConditionProgrammed))
    if cond != nil && cond.ObservedGeneration == generation {

    @kaito-pr-agent
    Copy link

    kaito-pr-agent bot commented Apr 15, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 626bb4f

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Rename finalizer to avoid conflicts

    Ensure uniqueness of finalizer names to avoid conflicts.

    pkg/common/objectmeta/objectmeta.go [37]

    -MetricsFinalizer = fleetNetworkingPrefix + "metrics-cleanup"
    +MetricsFinalizer = fleetNetworkingPrefix + "traffic-manager-metrics-cleanup"
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion ensures uniqueness of finalizer names, which helps avoid conflicts with other controllers or extensions using similar finalizers.

    Medium

    Previous suggestions

    Suggestions up to commit 777bb9d
    CategorySuggestion                                                                                                                                    Impact
    General
    Handle potential errors during metric emission

    Ensure the metric emission logic handles potential errors gracefully.

    pkg/controllers/hub/trafficmanagerprofile/controller.go [125]

    -defer emitTrafficManagerProfileStatusMetric(profile)
    +defer func() {
    +    if err := emitTrafficManagerProfileStatusMetric(profile); err != nil {
    +        klog.ErrorS(err, "Failed to emit traffic manager profile status metric", "trafficManagerProfile", profileKRef)
    +    }
    +}()
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion improves the robustness of the code by adding error handling for the metric emission logic, which is important for maintaining the reliability of the application.

    Medium
    Suggestions up to commit 777bb9d
    CategorySuggestion                                                                                                                                    Impact
    General
    Add conditional check for status conditions before emitting metrics

    Ensure the metric emission logic handles cases where the profile status might not
    have changed.

    pkg/controllers/hub/trafficmanagerprofile/controller.go [125]

    -defer emitTrafficManagerProfileStatusMetric(profile)
    +defer func() {
    +    if len(profile.Status.Conditions) > 0 {
    +        emitTrafficManagerProfileStatusMetric(profile)
    +    }
    +}()
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion ensures that the metric emission logic only runs if there are status conditions present, preventing unnecessary metric emissions when the profile status hasn't changed.

    Medium

    @codecov
    Copy link

    codecov bot commented Apr 16, 2025

    Codecov Report

    Attention: Patch coverage is 71.73913% with 13 lines in your changes missing coverage. Please review.

    Project coverage is 80.49%. Comparing base (677e344) to head (626bb4f).
    Report is 1 commits behind head on main.

    Files with missing lines Patch % Lines
    ...ontrollers/hub/trafficmanagerprofile/controller.go 71.73% 10 Missing and 3 partials ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##             main     #288      +/-   ##
    ==========================================
    + Coverage   80.30%   80.49%   +0.18%     
    ==========================================
      Files          29       29              
      Lines        4032     4065      +33     
    ==========================================
    + Hits         3238     3272      +34     
    + Misses        630      629       -1     
      Partials      164      164              

    ☔ 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.

    @zhiying-lin zhiying-lin merged commit ae3b0aa into Azure:main Apr 17, 2025
    11 of 12 checks passed
    @zhiying-lin zhiying-lin deleted the add-metrics branch April 17, 2025 09:31
    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.

    2 participants