chore: correlation for worker logger #710
Conversation
lzap
force-pushed
the
sql-logging
branch
2 times, most recently
from
0f682a6 to
aba6075
Compare
|
This is weird, unit tests are failing locally for me. Not on CI tho :-) |
|
/retest |
|
Huh, I've started reviewing and then decided to actually learn about tracing bit more. Let's discuss tomorrow? I guess I'd prefer a meeting, I'm too deep and in need of second brain xD |
|
Found a typo, rebased. |
| principal := identity.Identity(ctx) | ||
| zx = zx.Str("org_id", principal.Identity.OrgID) | ||
| zx = zx.Str("account_number", principal.Identity.AccountNumber) | ||
| return zx | ||
| })) |
There was a problem hiding this comment.
This hunk ensures that all pgx logging has a proper context and will appear in Kibana correctly.
| if val == nil { | ||
| return Principal{} | ||
| } | ||
| return val.(Principal) |
There was a problem hiding this comment.
The Get from middleware project does not handle nil correctly, so this is a copy.
|
Ah test do fail locally on the |
| Logger()) | ||
| ctx = logger.WithContext(ctx) | ||
| return ctx, logger | ||
| } |
There was a problem hiding this comment.
Just saw your PR and realized some of this code should live in worker.contextLogger instead. Moving.
|
I just moved most of the logging/context handling into worker/ package where it belongs. Only reservation ID is in the jobs/ package now. |
Signed-off-by: Lukas Zapletal <[email protected]> Signed-off-by: Lukáš Zapletal <[email protected]>
|
One additional change: |
| @@ -96,7 +83,11 @@ func (w *RedisWorker) Enqueue(ctx context.Context, job *Job) error { | |||
| } | |||
| } | |||
|
|
|||
| logger := loggerWithJob(ctx, job) | |||
There was a problem hiding this comment.
Why did you removed the function? 🤔
There was a problem hiding this comment.
Yeah there is replacement now, only for two cases there is not yet enough information so I replaced it with plain code.
| ctx = contextLogger(ctx, job) | ||
| ctx, logger := contextLogger(origCtx, job) | ||
| defer recoverAndLog(ctx) | ||
| logger.Info().Msgf("Dequeued job %s %s from Redis", job.Type.String(), job.ID) |
There was a problem hiding this comment.
We are now not logging the attributes on Dequeue, is that intentional?
There was a problem hiding this comment.
We do, it is in the contextLogger function now (every log entry has it now).
We do not yet, but we should keep it in mind, there might be some sensitive data in the future. |
|
Hmm unfortunately, I don’t see query parameters being logged at all. Other than that it works and with your patch the trace will be actually correct :-) |
During investigation of a problem, I noticed that worker is missing jobID from all pgx log messages therefore you will not see some records which are useful. This fixes it and unifies how context (and logger) is created across jobs. It also adds trace/edge IDs so logs can be correlated across api/worker to see the whole workflow of a reservation.
I cannot figure out why tests are failing, it is too late my brain does not work anymore. Will take a look tomorrow.