From f9d61f2799047ef7baa999b0e88323492b4430ca Mon Sep 17 00:00:00 2001 From: Liang Mei Date: Wed, 1 Dec 2021 10:20:23 -0800 Subject: [PATCH] Add debug log for all errors received from RecordActivityHeartbeat (#657) --- internal/activity.go | 2 +- internal/internal_activity.go | 9 +++------ internal/internal_task_handlers.go | 5 +++++ internal/internal_task_handlers_test.go | 10 ++++++++-- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/activity.go b/internal/activity.go index b0e43e8f4..111e20452 100644 --- a/internal/activity.go +++ b/internal/activity.go @@ -193,7 +193,7 @@ func GetWorkerStopChannel(ctx context.Context) <-chan struct{} { // TODO: we don't have a way to distinguish between the two cases when context is canceled because // context doesn't support overriding value of ctx.Error. // TODO: Implement automatic heartbeating with cancellation through ctx. -// details - the details that you provided here can be seen in the worflow when it receives TimeoutError, you +// details - the details that you provided here can be seen in the workflow when it receives TimeoutError, you // can check error TimeoutType()/Details(). func RecordActivityHeartbeat(ctx context.Context, details ...interface{}) { getActivityOutboundInterceptor(ctx).RecordHeartbeat(ctx, details...) diff --git a/internal/internal_activity.go b/internal/internal_activity.go index e6e7c9cc6..dbcc589ec 100644 --- a/internal/internal_activity.go +++ b/internal/internal_activity.go @@ -394,7 +394,7 @@ func (a *activityEnvironmentInterceptor) RecordHeartbeat(ctx context.Context, de } var data *commonpb.Payloads var err error - // We would like to be a able to pass in "nil" as part of details(that is no progress to report to) + // We would like to be able to pass in "nil" as part of details(that is no progress to report to) if len(details) > 1 || (len(details) == 1 && details[0] != nil) { data, err = encodeArgs(getDataConverterFromActivityCtx(ctx), details) if err != nil { @@ -402,11 +402,8 @@ func (a *activityEnvironmentInterceptor) RecordHeartbeat(ctx context.Context, de } } - err = a.env.serviceInvoker.Heartbeat(ctx, data, false) - if err != nil { - log := GetActivityLogger(ctx) - log.Debug("RecordActivityHeartbeat with error", tagError, err) - } + // Heartbeat error is logged inside ServiceInvoker.internalHeartBeat + _ = a.env.serviceInvoker.Heartbeat(ctx, data, false) } func (a *activityEnvironmentInterceptor) HasHeartbeatDetails(ctx context.Context) bool { diff --git a/internal/internal_task_handlers.go b/internal/internal_task_handlers.go index 3a9eccd34..245d4b2b2 100644 --- a/internal/internal_task_handlers.go +++ b/internal/internal_task_handlers.go @@ -1694,6 +1694,11 @@ func (i *temporalInvoker) internalHeartBeat(ctx context.Context, details *common } } + if err != nil { + logger := GetActivityLogger(ctx) + logger.Debug("RecordActivityHeartbeat with error", tagError, err) + } + // This error won't be returned to user check RecordActivityHeartbeat(). return isActivityCanceled, err } diff --git a/internal/internal_task_handlers_test.go b/internal/internal_task_handlers_test.go index 6e949f48e..31cf759b6 100644 --- a/internal/internal_task_handlers_test.go +++ b/internal/internal_task_handlers_test.go @@ -1398,7 +1398,10 @@ func (t *TaskHandlersTestSuite) TestHeartBeat_NilResponseWithError() { nil, "Test_Temporal_Invoker", mockService, tally.NoopScope, func() {}, 0, make(chan struct{}), t.namespace) - heartbeatErr := temporalInvoker.Heartbeat(context.Background(), nil, false) + ctx, err := newActivityContext(context.Background(), nil, &activityEnvironment{serviceInvoker: temporalInvoker, logger: t.logger}) + t.NoError(err) + + heartbeatErr := temporalInvoker.Heartbeat(ctx, nil, false) t.NotNil(heartbeatErr) t.IsType(&serviceerror.NotFound{}, heartbeatErr, "heartbeatErr must be of type NotFound.") } @@ -1416,7 +1419,10 @@ func (t *TaskHandlersTestSuite) TestHeartBeat_NilResponseWithNamespaceNotActiveE nil, "Test_Temporal_Invoker", mockService, tally.NoopScope, cancelHandler, 0, make(chan struct{}), t.namespace) - heartbeatErr := temporalInvoker.Heartbeat(context.Background(), nil, false) + ctx, err := newActivityContext(context.Background(), nil, &activityEnvironment{serviceInvoker: temporalInvoker, logger: t.logger}) + t.NoError(err) + + heartbeatErr := temporalInvoker.Heartbeat(ctx, nil, false) t.NotNil(heartbeatErr) t.IsType(&serviceerror.NamespaceNotActive{}, heartbeatErr, "heartbeatErr must be of type NamespaceNotActive.") t.True(called)