fix(router): avoid settings logger from request due to race conditions

[Pitasi]

This fixes failing code_cov tests due to a concurrency problem that I did point out here: #20 (comment).

[gsora]

Hmm, why CI is all red? 🤔

[Pitasi]

Hmm, why CI is all red? 🤔

because it's blocked by this change: EmerisHQ/emeris-utils#37 I don't know the best practice of managing this kind of changes across multiple repositories :/

[sgerogia]

Hmm, why CI is all red? 🤔

because it's blocked by this change: allinbits/emeris-utils#37 I don't know the best practice of managing this kind of changes across multiple repositories :/

@antonio The allinbits/emeris-utils#37 PR needs to be marked with minor or patch label (depending on compatibility).
This will auto-generate a version, preventing accidental downstream breaks.
See for example some closed PRs

[0xmuralik]

Now that setLoggerFromContext is removed, I think we need to use the AddCorrelationIDToLogger in deps.LogError and deps.WriteError too, so that the correlation id's get included there.

[Pitasi]

Now that setLoggerFromContext is removed, I think we need to use the AddCorrelationIDToLogger in deps.LogError and deps.WriteError too, so that the correlation id's get included there.

The deps logger already contains the correlation id, right? It should be set by the CorrelationIDMiddleware you wrote.

[0xmuralik]

Now that setLoggerFromContext is removed, I think we need to use the AddCorrelationIDToLogger in deps.LogError and deps.WriteError too, so that the correlation id's get included there.

The deps logger already contains the correlation id, right? It should be set by the CorrelationIDMiddleware you wrote.

Could run this locally and check if the logRequest and deps include correlationIds?

[Pitasi]

@SpideyPool192 I see your point. I didn't understand that the middleware was setting a logger inside the gin.context, but we weren't using it 😅

So my last commit: fc25f38 (#70) should fix that. Let me know if it seems right to you.

Eventually in my head the whole deps package will go away, since all the deps are "static" (aka not depending on the particular HTTP request, they are the same for everyone), expect for the logger (so only the logger will be treated differently). But that's for the future :)

[0xmuralik]

So my last commit: fc25f38 (#70) should fix that. Let me know if it seems right to you.

We already have a logging.GetLoggerFromContext in utils. Can we refactor that if necessary and use it here?

[Pitasi]

So my last commit: fc25f38 (#70) should fix that. Let me know if it seems right to you.

We already have a logging.GetLoggerFromContext in utils. Can we refactor that if necessary and use it here?

amazing, thanks for pointing it out! I removed my method which basically was the same in favour of the utils one.

[0xmuralik]

LGTM.