potiuk commented on PR #68355:
URL: https://github.com/apache/airflow/pull/68355#issuecomment-4694871963
Nice cleanup — the structured `buildFetchedJobLogAttrs` is the right
approach, and the regression test asserting the token is still returned but
never logged is exactly what this needs. 👍
One thing I think this misses: there's a third site that still logs the
workload verbatim, at Debug level —
```go
// go-sdk/edge/worker.go
case workload := <-workloadsChan:
w.logger.Debug("Got allocation", "workload", workload)
```
`workload.ExecuteTaskWorkload` carries the `Token`, and
`ExecuteTaskWorkload` has no `LogValue()`, so with debug logging enabled the
token still ends up in the logs there. Given the PR title is "stop logging
fetched edge workloads verbatim", it'd be good to cover this one too.
Two options:
1. Log only safe identifiers here as well (same shape as the new helper), or
2. Add a redacting `slog.LogValuer` (`LogValue()`) on `ExecuteTaskWorkload`
that masks `Token` — so any current or future log site that logs the workload
is safe by construction, not just the ones we remember to fix.
I'd lean toward (2) as the more durable fix, optionally combined with (1)
for this site. Minor nit: `buildFetchedJobLogAttrs` reconstructs an
`EdgeJobFetched` from `resp` just to read a few fields — you could pass `resp`
directly.
Thanks for the careful work on this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
