Skip to content

Commit

Permalink
[DEVHAS-611] Update Component create metrics for better data (#441)
Browse files Browse the repository at this point in the history
* Update metrics only if current err state is different to the prev

Signed-off-by: Maysun J Faisal <[email protected]>

* Remove metrics tracking for Gitops due to future commitments

Signed-off-by: Maysun J Faisal <[email protected]>

---------

Signed-off-by: Maysun J Faisal <[email protected]>
  • Loading branch information
maysunfaisal authored Feb 7, 2024
1 parent 632e05e commit da050c7
Show file tree
Hide file tree
Showing 12 changed files with 637 additions and 411 deletions.
118 changes: 43 additions & 75 deletions controllers/component_controller.go

Large diffs are not rendered by default.

314 changes: 157 additions & 157 deletions controllers/component_controller_test.go

Large diffs are not rendered by default.

30 changes: 29 additions & 1 deletion controllers/component_controller_unit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,7 @@ func TestCheckForCreateReconcile(t *testing.T) {
name string
component appstudiov1alpha1.Component
wantCreateReconcile bool
wantErrMsg string
}{
{
name: "Component with Create Condition",
Expand All @@ -595,6 +596,29 @@ func TestCheckForCreateReconcile(t *testing.T) {
},
wantCreateReconcile: true,
},
{
name: "Component with Create Condition in error state",
component: appstudiov1alpha1.Component{
ObjectMeta: metav1.ObjectMeta{
Name: "comp1",
},
Spec: appstudiov1alpha1.ComponentSpec{
ComponentName: "comp1",
},
Status: appstudiov1alpha1.ComponentStatus{
Conditions: []metav1.Condition{
{
Type: "Created",
Status: metav1.ConditionFalse,
Reason: "Error",
Message: "Component create failed",
},
},
},
},
wantCreateReconcile: true,
wantErrMsg: "Component create failed",
},
{
name: "Component with no Conditions",
component: appstudiov1alpha1.Component{
Expand Down Expand Up @@ -640,11 +664,15 @@ func TestCheckForCreateReconcile(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := checkForCreateReconcile(tt.component)
got, gotErrMsg := checkForCreateReconcile(tt.component)

if tt.wantCreateReconcile != got {
t.Errorf("TestCheckForCreateReconcile error: expected %v, got %v", tt.wantCreateReconcile, got)
}

if tt.wantErrMsg != gotErrMsg {
t.Errorf("TestCheckForCreateReconcile error: expected error msg %v, got %v", tt.wantErrMsg, gotErrMsg)
}
})
}

Expand Down
34 changes: 34 additions & 0 deletions pkg/github/errors.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
//
// Copyright 2024 Red Hat, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package github

// GitHubUserErr is a user related error for the GitHub related operations
type GitHubUserErr struct {
Err string
}

func (e *GitHubUserErr) Error() string {
return e.Err
}

// GitHubSystemErr is a system related error for the GitHub related operations
type GitHubSystemErr struct {
Err string
}

func (e *GitHubSystemErr) Error() string {
return e.Err
}
7 changes: 2 additions & 5 deletions pkg/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,12 @@ func (g *GitHubClient) GetDefaultBranchFromURL(repoURL string, ctx context.Conte
func (g *GitHubClient) GetBranchFromURL(repoURL string, ctx context.Context, branchName string) (*github.Branch, error) {
repoName, orgName, err := GetRepoAndOrgFromURL(repoURL)
if err != nil {
// User error - so increment the "success" metric - since we're tracking only system errors
metrics.ComponentCreationSucceeded.Inc()
return nil, err
return nil, &GitHubUserErr{Err: err.Error()}
}

branch, _, err := g.Client.Repositories.GetBranch(ctx, orgName, repoName, branchName, false)
if err != nil || branch == nil {
metrics.ComponentCreationFailed.Inc()
return nil, fmt.Errorf("failed to get branch %s from repo %s under %s, error: %v", branchName, repoName, orgName, err)
return nil, &GitHubSystemErr{Err: fmt.Sprintf("failed to get branch %s from repo %s under %s, error: %v", branchName, repoName, orgName, err)}
}

return branch, nil
Expand Down
15 changes: 0 additions & 15 deletions pkg/github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,9 +504,6 @@ func TestGetBranchFromURL(t *testing.T) {
Client: GetMockedClient(),
}

beforeCreateSuccess := testutil.ToFloat64(metrics.ComponentCreationSucceeded)
beforeCreateFailed := testutil.ToFloat64(metrics.ComponentCreationFailed)

t.Run(tt.name, func(t *testing.T) {
branch, err := mockedClient.GetBranchFromURL(tt.repoURL, context.Background(), tt.branchName)

Expand All @@ -516,18 +513,6 @@ func TestGetBranchFromURL(t *testing.T) {
if !tt.wantErr && *branch.Name != tt.branchName {
t.Errorf("TestGetBranchFromURL() error: expected %v got %v", tt.branchName, *branch.Name)
}

if tt.wantErr {
if tt.name == "Unparseable URL" {
if testutil.ToFloat64(metrics.ComponentCreationSucceeded) <= beforeCreateSuccess {
t.Errorf("TestGetBranchFromURL() expected metric 'ComponentCreationSucceeded' to be incremented, beforeCreateSuccess %v and testutil.ToFloat64(metrics.ComponentCreationSucceeded) %v", beforeCreateSuccess, testutil.ToFloat64(metrics.ComponentCreationSucceeded))
}
} else if tt.name == "Simple repo name" {
if testutil.ToFloat64(metrics.ComponentCreationFailed) <= beforeCreateFailed {
t.Errorf("TestGetBranchFromURL() expected metric 'ComponentCreationFailed' to be incremented, beforeCreateFailed %v and testutil.ToFloat64(metrics.ComponentCreationFailed) %v", beforeCreateFailed, testutil.ToFloat64(metrics.ComponentCreationFailed))
}
}
}
})
}
}
60 changes: 60 additions & 0 deletions pkg/metrics/application.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
//
// Copyright 2024 Red Hat, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package metrics

import "github.com/prometheus/client_golang/prometheus"

var (
ApplicationDeletionTotalReqs = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_deletion_total",
Help: "Number of application deletion requests processed",
},
)
ApplicationDeletionFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_failed_deletion_total",
Help: "Number of failed application deletion requests",
},
)

ApplicationDeletionSucceeded = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_successful_deletion_total",
Help: "Number of successful application deletion requests",
},
)

ApplicationCreationTotalReqs = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_creation_total",
Help: "Number of application creation requests processed",
},
)
ApplicationCreationFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_failed_creation_total",
Help: "Number of failed application creation requests",
},
)

ApplicationCreationSucceeded = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_application_successful_creation_total",
Help: "Number of successful application creation requests",
},
)
)
103 changes: 103 additions & 0 deletions pkg/metrics/component.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
//
// Copyright 2024 Red Hat, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package metrics

import (
"strings"

"github.com/prometheus/client_golang/prometheus"
)

var (
componentCreationTotalReqs = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_creation_total",
Help: "Number of component creation requests processed",
},
)
componentCreationFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_failed_creation_total",
Help: "Number of failed component creation requests",
},
)

componentCreationSucceeded = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_successful_creation_total",
Help: "Number of successful component creation requests",
},
)

ComponentDeletionTotalReqs = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_deletion_total",
Help: "Number of component deletion requests processed",
},
)
ComponentDeletionFailed = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_failed_deletion_total",
Help: "Number of failed component deletion requests",
},
)

ComponentDeletionSucceeded = prometheus.NewCounter(
prometheus.CounterOpts{
Name: "has_component_successful_deletion_total",
Help: "Number of successful component deletion requests",
},
)
)

// IncrementComponentCreationFailed increments the component creation failed metric.
// Pass in the new error to update the metric, otherwise it will be ignored.
func IncrementComponentCreationFailed(oldError, newError string) {
if newError != "" && (oldError == "" || !strings.Contains(oldError, newError)) {
// pair the componentCreationTotalReqs counter with componentCreationFailed because
// we dont want a situation where we increment componentCreationTotalReqs in the
// beginning of a reconcile, and we skip the componentCreationFailed metric because
// the errors are the same. Otherwise we will have a situation where neither the success
// nor the fail metric is increasing but the total request count is increasing.
componentCreationTotalReqs.Inc()
componentCreationFailed.Inc()
}
}

func GetComponentCreationFailed() prometheus.Counter {
return componentCreationFailed
}

// IncrementComponentCreationSucceeded increments the component creation succeeded metric.
func IncrementComponentCreationSucceeded(oldError, newError string) {
if oldError == "" || newError == "" || !strings.Contains(oldError, newError) {
// pair the componentCreationTotalReqs counter with componentCreationSucceeded because
// we dont want a situation where we increment componentCreationTotalReqs in the
// beginning of a reconcile, and we skip the componentCreationSucceeded metric because
// the errors are the same. Otherwise we will have a situation where neither the success
// nor the fail metric is increasing but the total request count is increasing.
componentCreationTotalReqs.Inc()
componentCreationSucceeded.Inc()
}
}

func GetComponentCreationSucceeded() prometheus.Counter {
return componentCreationSucceeded
}

func GetComponentCreationTotalReqs() prometheus.Counter {
return componentCreationTotalReqs
}
93 changes: 93 additions & 0 deletions pkg/metrics/component_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
//
// Copyright 2024 Red Hat, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package metrics

import (
"testing"

"github.com/prometheus/client_golang/prometheus/testutil"
)

func TestComponentMetricsIncrement(t *testing.T) {

tests := []struct {
name string
oldErr string
newErr string
expectSuccessIncremented bool
expectFailureIncremented bool
}{
{
name: "no errors",
oldErr: "",
newErr: "",
expectSuccessIncremented: true,
expectFailureIncremented: false,
},
{
name: "no old error, new error",
oldErr: "",
newErr: "error",
expectSuccessIncremented: true,
expectFailureIncremented: true,
},
{
name: "old error, no new error",
oldErr: "error",
newErr: "",
expectSuccessIncremented: true,
expectFailureIncremented: false,
},
{
name: "old error, new error - same error",
oldErr: "error",
newErr: "error",
expectSuccessIncremented: false,
expectFailureIncremented: false,
},
{
name: "old error, new error - different error",
oldErr: "error",
newErr: "error 2",
expectSuccessIncremented: true,
expectFailureIncremented: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
beforeCreateSuccess := testutil.ToFloat64(componentCreationSucceeded)
beforeCreateFailed := testutil.ToFloat64(componentCreationFailed)

IncrementComponentCreationSucceeded(tt.oldErr, tt.newErr)

if tt.expectSuccessIncremented && testutil.ToFloat64(componentCreationSucceeded) <= beforeCreateSuccess {
t.Errorf("TestComponentMetricsIncrement error: expected component create success metrics to be incremented but was not incremented")
} else if !tt.expectSuccessIncremented && testutil.ToFloat64(componentCreationSucceeded) > beforeCreateSuccess {
t.Errorf("TestComponentMetricsIncrement error: expected component create success metrics not to be incremented but it was incremented")
}

IncrementComponentCreationFailed(tt.oldErr, tt.newErr)

if tt.expectFailureIncremented && testutil.ToFloat64(componentCreationFailed) <= beforeCreateFailed {
t.Errorf("TestComponentMetricsIncrement error: expected component create failed metrics to be incremented but was not incremented")
} else if !tt.expectFailureIncremented && testutil.ToFloat64(componentCreationFailed) > beforeCreateFailed {
t.Errorf("TestComponentMetricsIncrement error: expected component create failed metrics not to be incremented but it was incremented")
}

})
}
}
Loading

0 comments on commit da050c7

Please sign in to comment.