Use logger interceptor in Temporal workers #1015
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1015 +/- ##
==========================================
- Coverage 53.13% 53.11% -0.02%
==========================================
Files 101 100 -1
Lines 5814 5810 -4
==========================================
- Hits 3089 3086 -3
+ Misses 2474 2471 -3
- Partials 251 253 +2 ☔ View full report in Codecov by Sentry. |
| @@ -22,6 +22,7 @@ import ( | |||
| "github.com/prometheus/client_golang/prometheus/promhttp" | |||
| "github.com/spf13/pflag" | |||
| "go.artefactual.dev/tools/log" | |||
| temporal_tools "go.artefactual.dev/tools/temporal" | |||
There was a problem hiding this comment.
If you like, you can enforce this alias here:
Lines 41 to 47 in 8aa0da8
There was a problem hiding this comment.
I don't like to enforce it.
cmd/enduro-a3m-worker/main.go
Outdated
| @@ -212,6 +213,9 @@ func main() { | |||
| EnableSessionWorker: true, | |||
| MaxConcurrentSessionExecutionSize: cfg.A3m.Capacity, | |||
| MaxConcurrentActivityExecutionSize: 1, | |||
| Interceptors: []temporalsdk_interceptor.WorkerInterceptor{ | |||
| temporal_tools.NewLoggerInterceptor(logger.WithName("a3m-worker")), | |||
There was a problem hiding this comment.
The logger was already named enduro-a3m-worker (using appName) so I believe that the name of this child logger would be enduro-a3m.worker.a3m-worker, is that what you want?
There was a problem hiding this comment.
Good point, maybe not appending anything works better in this case.
| @@ -57,7 +51,8 @@ func (a *UploadTransferActivity) Execute( | |||
| ctx context.Context, | |||
| params *UploadTransferActivityParams, | |||
| ) (*UploadTransferActivityResult, error) { | |||
| a.logger.V(1).Info("Execute UploadTransferActivity", "SourcePath", params.SourcePath) | |||
| logger := temporal_tools.GetLogger(ctx) | |||
| logger.V(1).Info("Execute UploadTransferActivity", "SourcePath", params.SourcePath) | |||
There was a problem hiding this comment.
A thought for the future: I've always seen this logging entry as unnecessary becuse the SDK produces a similar event automatically if I remember correctly, it does not log local variables or parameters but inputs are available in the Temporal workflow history anyways.
There was a problem hiding this comment.
I said the same in artefactual-sdps/temporal-activities#3 (comment). But @djjuhasz likes it.
There was a problem hiding this comment.
@sevein I find the log entry helpful for debugging failures where Temporal schedules the activity (so it shows up in the workflow history) but the worker doesn't actually run the Execute() method. Echoing the parameters is probably unnecessary in that case though.
There was a problem hiding this comment.
Perhaps that can be done from the worker interceptor too (ActivityInboundInterceptor), I will investigate.
Follow https://enduro.readthedocs.io/dev-manual/logging recommendations. - Use `go.artefactual.dev/tools/temporal` to set the clients logger. - Use replay-aware Temporal SDK logger from processing workflow. - Add logger interceptor to all workers. - Use `go.artefactual.dev/tools/temporal` `GetLogger` in activities. - Remove logger usage from local activities.
jraddaoui
force-pushed
the
dev/issue-1000-logger-interceptor
branch
from
8ca0b45 to
0ecec2a
Compare
|
Thanks @sevein! |
Follow https://enduro.readthedocs.io/dev-manual/logging recommendations.
go.artefactual.dev/tools/temporalto set the clients logger.go.artefactual.dev/tools/temporalGetLoggerin activities.Refs #1000.