docs: auditorias de cobertura de logs do serviço - #205
Draft
nicholas-maestrello wants to merge 3 commits into
Draft
docs: auditorias de cobertura de logs do serviço#205nicholas-maestrello wants to merge 3 commits into
nicholas-maestrello wants to merge 3 commits into
Conversation
Registra duas auditorias independentes dos caminhos de erro de node/ no commit 508bff6, para que as decisões deliberadas de logging (como trocar log por métrica em caminho de alto volume) não sejam reabertas a cada revisão, e para dar rastreabilidade aos caminhos de falha silenciosa que ainda estão abertos. Co-authored-by: Cursor <[email protected]>
|
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
|
Beep boop 🤖 Thank you so much for keeping our documentation up-to-date ❤️ |
O 89% da primeira auditoria mistura duas unidades no denominador: conta os caminhos cobertos um por um, mas os achados como linhas de tabela agrupadas, que somam 21 locais e não 10. Normalizada, ela daria ~80%. Registra a aritmética para que a queda de 89% para 77% não seja lida como regressão de cobertura. Co-authored-by: Cursor <[email protected]>
Registra a convenção de sufixar o arquivo com o commit auditado quando houver mais de uma auditoria no mesmo dia. Co-authored-by: Cursor <[email protected]>
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
O que é
Adiciona
docs/log-coverage-audits/, com duas auditorias independentes dos caminhos de erro denode/feitas sobre o commit508bff6, mais um README com o histórico e as convenções.São só documentos — nenhuma mudança de código neste PR. A intenção é dupla:
.catch(() => null)dedirectives/withSession.ts:26, que foi trocado porSessionMetricjustamente por volume de log.Achados
A camada de autenticação (
directives/helper.tse os quatro directives de acesso) está bem instrumentada: todo caminho de negação logawarncom o conjunto completo demetricFields. O mesmo vale para praticamente todo resolver de mutation.O achado mais relevante é que
setProfilenão registra o erro quando uma das chamadas a clientes externos falha. No commit auditado, seis chamadas dentro dele não tinham.catch()e a função não tinhatry/catchexterno — uma indisponibilidade dob2b-organizations-graphqlderrubava todosetProfilesem uma linha de log do serviço.Atenção: o achado principal mudou de forma no master
O commit auditado está 35 commits atrás do
master, que reescreveuRoutes/index.ts. Reconferi antes de abrir o PR e registrei na seção "Nota sobre o master":setProfiledomasteragora envolve cada chamada emtimedSetProfile(Routes/index.ts:49). Ocatchdesse wrapper (:75) loga emlogger.debugcomfailed: true, sem o objetoerror, e relança. O passo que falhou ficou identificável; o erro em si, não — edebugnormalmente é filtrado em produção.setProfilecontinua semtry/catchno nível superior, eRoutes.appSettings/services/appSettingsCache.ts:22continuam sem guarda.Ou seja, o achado continua aberto no
master, só com outra forma. A correção mais direta hoje é ocatchdotimedSetProfilelogarlogger.error({ error, message: 'setProfile.stepFailed', step })antes dothrow, mais otry/catchexterno.Os números de linha dos dois relatórios valem para
508bff6e estão declarados como tal no cabeçalho de cada um.Sobre a diferença de score (89% vs. 77%)
Não é regressão de cobertura, e não é só granularidade. O lado coberto das duas auditorias praticamente coincide (85 e 86) — as duas acharam o mesmo conjunto de caminhos instrumentados.
A primeira auditoria mistura duas unidades no denominador: fechou
85 + 10 = 95, contando os cobertos um por um mas os achados como linhas de tabela agrupadas. Várias daquelas linhas cobrem vários locais (oLicenseManageré uma linha com 6 catches, ogetUseré uma linha com o.catchmais 3 callers), e expandidas somam 21 locais, não 10. Normalizada para a mesma unidade, ela daria ~80%, não 89%.A seção "Reconciliação dos scores" do relatório de revisão mostra a aritmética fechando: 21 locais, menos 1 inválido, mais os 6 dos achados novos, dá os 26 daqui. Deixei isso explícito para que a próxima auditoria não leia a queda como piora.
Notas de revisão
Queries/Users.ts:592como o achado mais grave, mas aquele branch é inalcançável (getUserByEmailsempre retorna um array de um elemento) e o caso de usuário não encontrado já é logado emgetActiveUserByEmail:168. Sobra apenas:607.Test plan
Não se aplica — mudança exclusivamente de documentação, sem alteração em
node/.