Skip to content

Commit

Permalink
[Disk Manager]: Fix incorrect grpc facade error reporting (#2469)
Browse files Browse the repository at this point in the history
We did not report errors except context cancellation in grpc facade.
  • Loading branch information
jkuradobery authored Nov 13, 2024
1 parent 1e5a496 commit 5dfbd00
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 1 deletion.
46 changes: 46 additions & 0 deletions cloud/disk_manager/internal/pkg/facade/facade_test/facade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@ package facade_test
import (
"testing"

"github.com/golang/protobuf/ptypes/empty"
"github.com/stretchr/testify/require"
disk_manager "github.com/ydb-platform/nbs/cloud/disk_manager/api"
"github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/facade/testcommon"
"github.com/ydb-platform/nbs/cloud/tasks/errors"
"google.golang.org/protobuf/proto"
)
Expand Down Expand Up @@ -48,3 +50,47 @@ func TestFacadeErrorDetails(t *testing.T) {
errorDetails1.Message = "ZYX"
check()
}

func TestFacadeShouldSendErrorMetrics(t *testing.T) {
ctx := testcommon.NewContext()
errorsCount := testcommon.GetCounter(
t,
"errors",
map[string]string{
"component": "grpc_facade",
"request": "DiskService.Create",
},
)
client, err := testcommon.NewClient(ctx)
require.NoError(t, err)
defer client.Close()

diskID := t.Name()
reqCtx := testcommon.GetRequestContext(t, ctx)
_, err = client.CreateDisk(reqCtx, &disk_manager.CreateDiskRequest{
Src: &disk_manager.CreateDiskRequest_SrcEmpty{
SrcEmpty: &empty.Empty{},
},
// Incorrect size to trigger errors
Size: 1,
Kind: disk_manager.DiskKind_DISK_KIND_SSD,
DiskId: &disk_manager.DiskId{
ZoneId: "zone-a",
DiskId: diskID,
},
})
require.Error(t, err)
newErrorsCount := testcommon.GetCounter(
t,
"errors",
map[string]string{
"component": "grpc_facade",
"request": "DiskService.Create",
},
)
require.GreaterOrEqual(
t,
newErrorsCount-errorsCount,
float64(1),
)
}
2 changes: 1 addition & 1 deletion cloud/disk_manager/internal/pkg/facade/interceptor.go
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ func (i *interceptor) intercept(
if err != nil {
logging.Warn(ctx, "%v failed, err=%v", requestName, err)
// Don't report errors when ctx is cancelled.
if ctx.Err() != nil {
if ctx.Err() == nil {
requestStats.onError(err)
}
}
Expand Down
54 changes: 54 additions & 0 deletions cloud/disk_manager/internal/pkg/facade/testcommon/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"time"

"github.com/golang/protobuf/ptypes/empty"
prometheus_client "github.com/prometheus/client_model/go"
"github.com/prometheus/common/expfmt"
"github.com/stretchr/testify/require"
disk_manager "github.com/ydb-platform/nbs/cloud/disk_manager/api"
internal_client "github.com/ydb-platform/nbs/cloud/disk_manager/internal/pkg/client"
Expand Down Expand Up @@ -646,3 +648,55 @@ func CheckErrorDetails(
require.Equal(t, message, errorDetails.Message)
}
}

////////////////////////////////////////////////////////////////////////////////

func GetCounter(t *testing.T, name string, labels map[string]string) float64 {
resp, err := http.Get(
fmt.Sprintf(
"http://localhost:%s/metrics/",
os.Getenv("DISK_MANAGER_RECIPE_DISK_MANAGER_MON_PORT"),
),
)
require.NoError(t, err)
defer resp.Body.Close()

parser := expfmt.TextParser{}
metricFamilies, err := parser.TextToMetricFamilies(resp.Body)
require.NoError(t, err)

retrievedMetrics, ok := metricFamilies[name]
require.True(t, ok)
for _, metricValue := range retrievedMetrics.GetMetric() {
if metricMatchesLabel(labels, metricValue) {
return metricValue.GetCounter().GetValue()
}
}

require.Failf(t, "No counter with name %s", name)
return 0
}

func metricMatchesLabel(
labels map[string]string,
metric *prometheus_client.Metric,
) bool {

metricLabels := make(map[string]string)
for _, label := range metric.GetLabel() {
metricLabels[label.GetName()] = label.GetValue()
}

for name, value := range labels {
foundValue, ok := metricLabels[name]
if !ok {
return false
}

if foundValue != value {
return false
}
}

return true
}
1 change: 1 addition & 0 deletions cloud/disk_manager/test/recipe/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ def start(argv):
)
disk_managers.append(disk_manager)
disk_manager.start()
set_env("DISK_MANAGER_RECIPE_DISK_MANAGER_MON_PORT", str(disk_managers[0].monitoring_port))

dataplane_disk_managers_count = 1
for _ in range(0, dataplane_disk_managers_count):
Expand Down
4 changes: 4 additions & 0 deletions cloud/disk_manager/test/recipe/disk_manager_launcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -472,3 +472,7 @@ def port(self):
@property
def server_config(self):
return self.__server_config

@property
def monitoring_port(self):
return self.__monitoring_port

0 comments on commit 5dfbd00

Please sign in to comment.