Skip to content

Commit eb79763

Browse files
refactor: [shard-distributor]Remove error logs from store level (#7492)
<!-- Describe what has changed in this PR --> **What changed?** Removing the "Store failed with internal error." and "Executor not found." from the store level. <!-- Tell your future self why have you made these changes --> **Why?** Since the error is returned in the call() method, returning the error message is handled in the higher level. <!-- How have you verified this change? Tested locally? Added a unit test? Checked in staging env? --> **How did you test it?** unit tests & local testing <!-- Assuming the worst case, what can be broken when deploying this change to production? --> **Potential risks** N/A <!-- Is it notable for release? e.g. schema updates, configuration or data migration required? If so, please mention it, and also update CHANGELOG.md --> **Release notes** Cleaning up logs. <!-- Is there any documentation updates should be made for config, https://cadenceworkflow.io/docs/operation-guide/setup/ ? If so, please open an PR in https://github.com/cadence-workflow/cadence-docs --> **Documentation Changes** --------- Signed-off-by: Gaziza Yestemirova <[email protected]>
1 parent fc05d75 commit eb79763

File tree

2 files changed

+10
-35
lines changed

2 files changed

+10
-35
lines changed

service/sharddistributor/store/wrappers/metered/base.go

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727

2828
"github.com/uber/cadence/common/clock"
2929
"github.com/uber/cadence/common/log"
30-
"github.com/uber/cadence/common/log/tag"
3130
"github.com/uber/cadence/common/metrics"
3231
"github.com/uber/cadence/service/sharddistributor/store"
3332
)
@@ -38,18 +37,11 @@ type base struct {
3837
timeSource clock.TimeSource
3938
}
4039

41-
func (p *base) updateErrorMetricPerNamespace(scope metrics.ScopeIdx, err error, scopeWithNamespaceTags metrics.Scope, logger log.Logger) {
42-
logger = logger.Helper()
43-
44-
switch {
45-
case errors.Is(err, store.ErrExecutorNotFound):
40+
func (p *base) updateErrorMetricPerNamespace(err error, scopeWithNamespaceTags metrics.Scope) {
41+
if errors.Is(err, store.ErrExecutorNotFound) {
4642
scopeWithNamespaceTags.IncCounter(metrics.ShardDistributorStoreExecutorNotFound)
47-
logger.Error("Executor not found.", tag.Error(err), tag.MetricScope(int(scope)))
48-
case errors.Is(err, store.ErrShardNotFound):
49-
// this is expected, so we don't want to log it
50-
default:
51-
logger.Error("Store failed with internal error.", tag.Error(err), tag.MetricScope(int(scope))) // int???
5243
}
44+
5345
scopeWithNamespaceTags.IncCounter(metrics.ShardDistributorStoreFailuresPerNamespace)
5446
}
5547

@@ -62,9 +54,8 @@ func (p *base) call(scope metrics.ScopeIdx, op func() error, tags ...metrics.Tag
6254
duration := p.timeSource.Since(before)
6355
metricsScope.RecordHistogramDuration(metrics.ShardDistributorStoreLatencyHistogramPerNamespace, duration)
6456

65-
logger := p.logger.Helper()
6657
if err != nil {
67-
p.updateErrorMetricPerNamespace(scope, err, metricsScope, logger)
58+
p.updateErrorMetricPerNamespace(err, metricsScope)
6859
}
6960
return err
7061
}

service/sharddistributor/store/wrappers/metered/metered_test.go

Lines changed: 6 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import (
1111

1212
"github.com/uber/cadence/common/clock"
1313
"github.com/uber/cadence/common/log"
14-
"github.com/uber/cadence/common/log/tag"
1514
"github.com/uber/cadence/common/metrics"
1615
"github.com/uber/cadence/common/types"
1716
"github.com/uber/cadence/service/sharddistributor/store"
@@ -31,33 +30,19 @@ func TestMeteredStore_GetHeartbeat(t *testing.T) {
3130
}
3231

3332
tests := []struct {
34-
name string
35-
setupMocks func(logger *log.MockLogger)
36-
error error
33+
name string
34+
error error
3735
}{
3836
{
39-
name: "Success",
40-
setupMocks: func(logger *log.MockLogger) {},
41-
error: nil,
37+
name: "Success",
38+
error: nil,
4239
},
4340
{
44-
name: "NotFound",
45-
setupMocks: func(logger *log.MockLogger) {
46-
logger.EXPECT().Error(
47-
"Executor not found.",
48-
[]tag.Tag{tag.Error(store.ErrExecutorNotFound), tag.MetricScope(int(metrics.ShardDistributorStoreGetHeartbeatScope))},
49-
).Times(1)
50-
},
41+
name: "NotFound",
5142
error: store.ErrExecutorNotFound,
5243
},
5344
{
54-
name: "Failure",
55-
setupMocks: func(logger *log.MockLogger) {
56-
logger.EXPECT().Error(
57-
"Store failed with internal error.",
58-
[]tag.Tag{tag.Error(&types.InternalServiceError{}), tag.MetricScope(int(metrics.ShardDistributorStoreGetHeartbeatScope))},
59-
).Times(1)
60-
},
45+
name: "Failure",
6146
error: &types.InternalServiceError{},
6247
},
6348
}
@@ -79,7 +64,6 @@ func TestMeteredStore_GetHeartbeat(t *testing.T) {
7964
mockLogger.EXPECT().Helper().Return(mockLogger).AnyTimes()
8065

8166
wrapped := NewStore(mockHandler, metricsClient, mockLogger, timeSource).(*meteredStore)
82-
tt.setupMocks(mockLogger)
8367

8468
gotHeartbeat, gotAssignedState, err := wrapped.GetHeartbeat(context.Background(), _testNamespace, _testExecutorID)
8569

0 commit comments

Comments
 (0)