Exibir (e retornar via API) a quantidade de qualificações positivas e negativas do conteúdo #1607
Conversation
Rafatcb
added
front
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
18e3661
to
fa9d54c
Compare
Eu modifiquei alguns testes para adicionar o Não implementei a UI pois ainda não teve um consenso dentre as opções oferecidas no issue #1370, mas essa é a "parte fácil" do PR. |
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
fa9d54c
to
59ea58c
Compare
|
Realizei um Editei o PR para atualizar os resultados do PS: Está quebrado em homologação, imagino que por causa da migração que não foi executada. |
There was a problem hiding this comment.
- Faz sentido mandarmos
tabcoins_debitcomo um valor <= 0 ou deveria ser um valor positivo (>= 0)?
Acho um pouco estranho retornar o valor negativo já que possui débito no nome, mas parece que facilitou a implementação, então não vejo problemas.
- Usei o
EXPLAIN ANALYZEe a funçãoget_current_balance_credit_debittrouxe um custo (aparentemente) relevante nas queries. Pode ser algo específico doEXPLAIN ANALYZE, que na prática não faça diferença. Precisa avaliar o tempo de execução com mais dados (homologação/produção). Enxergam alguma melhoria para as queries alteradas?
Precisamos pensar em como melhorar isso. Talvez até mudar algo na modelagem.
Uma opção para "contornar" esse custo seria calcular os votos sob demanda, realizando uma requisição separada. Na UI atual, essa requisição seria disparada ao realizar o
hovernos votos.
Principalmente por ser custoso, precisa ter cache, e como seria um cache diferente dos dados da página estática, ficaria inconsistente com muita frequência.
b434a45: retorno ao buscar um conteúdo específico (GET /contents/[username]/[slug],GET /contents/[username]/[slug]/parenteGET /contents/[username]/[slug]/root) com teste específico. Defini se deve retornar com base novalues.limit === 1, se ter uma sugestão melhor, pode falar.
Se conseguirmos melhorar o desempenho, acho que compensaria sempre retornar o crédito e débito e mostrar isso também na lista de conteúdos.
Acham melhor criar um novo teste específico nos arquivos para testar esses valores de
tabcoins_creditetabcoins_debit?
Acho que compensa um teste específico.
Está quebrado em homologação, imagino que por causa da migração que não foi executada.
Quando estiver tudo definido, vamos precisar quebrar o PR em dois, e a migration fica no primeiro PR. Assim não quebra nada, principalmente em produção.
models/content.js
Outdated
| @@ -120,7 +120,13 @@ async function findAll(values = {}, options = {}) { | |||
| contents.path, | |||
| users.username as owner_username, | |||
| content_window.total_rows, | |||
| get_current_balance('content:tabcoin', contents.id) as tabcoins, | |||
| ${ | |||
There was a problem hiding this comment.
Será que não compensa retornar os créditos e débitos mesmo quando está devolvendo a lista de conteúdos?
O Tooltip com a mesma informação pode aparecer na lista de conteúdos e no conteúdo em si.
There was a problem hiding this comment.
Eu havia removido tanto pela questão do custo maior quanto pela falta de espaço para exibir essa informação. Um Tooltip resolveria o segundo ponto.
Se descobrirmos como resolver o primeiro, podemos devolver também nas listas de conteúdos.
Enquanto eu avaliava o issue #1370 (relacionado à este PR), eu acabei olhando o #1388, tanto que fiz esse comentário #1388 (comment) com alguns achados sobre o cache de TabCoins. Pensei que conseguiria retornar o Eu ainda não consigo ter certeza sobre o impacto de desempenho apenas com o |
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
59ea58c
to
83e0e87
Compare
Alterações no códigoAtualizei os testes para criar um teste novo específico sobre o comportamento adicionado (o retorno de Análise de desempenhoCriei um banco com 227 mil usuários, 48 mil conteúdos e 114 mil votos (542 mil registros em Para analisar os endpoints, executei cada query 5 vezes consecutivas diretamente no banco de dados (pelo pgAdmin4) e anotei os tempos. Isso não é nada "científico", mas dá a base de comparação necessária para entendermos quais queries precisam de atenção. Ordenei os tempos na tabela de resultados do melhor tempo de cada para o pior de cada. Também disponibilizei o ❌
|
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 51ms | 233ms | +356% |
| 53ms | 236ms | +345% |
| 73ms | 242ms | +231% |
| 73ms | 249ms | +241% |
| 83ms | 269ms | +224% |
Explain: get_current_balance e get_current_balance_credit_debit.
Essa query precisa de melhorias.
⚠️ GET /api/v1/contents/[username]/[slug]/children
Primeiro eu realizei um teste com 27 comentários na publicação raiz em diferentes níveis (cenário "ok"), mas não teve impacto significativo:
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 59ms | 62ms | +5% |
| 66ms | 65ms | -1% |
| 73ms | 78ms | +7% |
| 74ms | 78ms | +5% |
| 119ms | 88ms | -26% |
Explain com 27 comentários na publicação: get_current_balance e get_current_balance_credit_debit.
Então, decidi fazer o mesmo teste com 2027 comentários na publicação raiz em diferentes níveis (cenário extremo):
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 171ms | 297ms | +73% |
| 188ms | 298ms | +58% |
| 208ms | 310ms | +49% |
| 212ms | 312ms | +47% |
| 220ms | 314ms | +43% |
Explain com 2027 comentários na publicação: get_current_balance e get_current_balance_credit_debit.
Interessante notar que o gargalo destas queries muda conforme a quantidade de comentários.
As publicações com mais comentários do TabNews provavelmente são Quem deseja acesso ao Repositório Privado do TabNews? (FECHADO) (804 comentários) e Tentando construir um pedaço de internet mais massa (747 comentários).
✅ POST /contents/[user]/[slug]/tabcoins
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 49ms | 45ms | -8% |
| 52ms | 48ms | -8% |
| 53ms | 49ms | -8% |
| 56ms | 64ms | +14% |
| 77ms | 79ms | +3% |
Explain: get_current_balance e get_current_balance_credit_debit.
A alteração dessa query não causou um impacto significativo.
Resumo / Conclusão
-
GET /api/v1/contents/[username]/[slug]precisa de melhorias tanto porque o impacto no desempenho foi significativo quanto porque é a query mais usada dentre as três. -
GET /api/v1/contents/[username]/[slug]/childrenpode receber melhorias. Com poucos comentários, não há impacto. Com muitos (centenas ou milhares, coisa que hoje provavelmente temos em menos de 5 publicações) o impacto é mais significativo.Uma das publicações com muitos comentários está referenciada em vários lugares, que é a Tentando construir um pedaço de internet mais massa, então provavelmente é acessada algumas vezes por dia, o que faz com que essa query também mereça atenção, mas não é tão urgente/impactante quanto a anterior.
-
POST /contents/[user]/[slug]/tabcoinsnão precisa de melhorias.
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
83e0e87
to
4f81846
Compare
Melhorias✅
|
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 50ms | 49ms | -2% |
| 53ms | 56ms | +6% |
| 53ms | 56ms | +6% |
| 56ms | 56ms | = |
| 63ms | 80ms | +27% |
Explain: get_current_balance e get_current_balance_credit_debit.
O ROWS 1 trouxe uma ótima melhoria 🎉 . Para ter certeza que está "excelente" seria necessário ainda mais registros, para que as queries demorassem mais, dando uma noção maior da diferença do tempo de execução.
⚠️ GET /api/v1/contents/[username]/[slug]/children
Teste com 27 comentários na publicação raiz em diferentes níveis (cenário "ok"). Acredito que a diferença de desempenho entre as queries não seja significativa nesse cenário, e os dois exemplos mais demorados abaixo do get_current_balance foram apenas valores discrepantes:
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 55ms | 55ms | = |
| 57ms | 57ms | = |
| 57ms | 57ms | = |
| 112ms | 60ms | -46% |
| 141ms | 62ms | -56% |
Explain com 27 comentários na publicação: get_current_balance e get_current_balance_credit_debit.
O mesmo teste com 2027 comentários na publicação raiz em diferentes níveis (cenário extremo):
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 186ms | 287ms | +54% |
| 191ms | 295ms | +54% |
| 199ms | 315ms | +58% |
| 213ms | 332ms | +56% |
| 265ms | 348ms | +31% |
Explain com 2027 comentários na publicação: get_current_balance e get_current_balance_credit_debit.
Parece que a mudança de ROWS 1 não trouxe um impacto relevante nesta query.
✅ POST /contents/[user]/[slug]/tabcoins
get_current_balance |
get_current_balance_credit_debit |
Diferença (%) |
|---|---|---|
| 44ms | 44ms | = |
| 46ms | 45ms | -2% |
| 47ms | 46ms | -2% |
| 51ms | 48ms | -6% |
| 52ms | 55ms | +6% |
Explain: get_current_balance e get_current_balance_credit_debit.
Continua ótimo, sem diferença significativa entre as duas queries.
Próximos passos
-
GET /api/v1/contents/[username]/[slug]/childrencontinua precisando de melhorias. Citando meu último comentário:Com muitos [comentários] (centenas ou milhares, coisa que hoje provavelmente temos em menos de 5 publicações) o impacto é mais significativo.
Uma das publicações com muitos comentários está referenciada em vários lugares, que é a Tentando construir um pedaço de internet mais massa, então provavelmente é acessada algumas vezes por dia, o que faz com que essa query também mereça atenção, mas não é tão urgente/impactante quanto a anterior.
Outro ponto menos importantes que pode ser avaliado ao resolver o ponto acima:
GET /api/v1/contents: com um bom desempenho, podemos retornar (e exibir) os créditos e débitos na lista de conteúdos (conforme comentado aqui).
Além disso, pensei em renomear a função de get_current_balance_credit_debit para get_content_balance_credit_debit, porque no commit 59ea58c a função passou a ter uma condição de considerar apenas eventos do tipo update:content:tabcoins para a contagem.
|
@Rafatcb, eu acho que De qualquer forma, pode adicionar o Sobre os próximos passos, não ficou claro se você planeja fazer no mesmo PR ou em outro. Eu tinha falado em separar a migration em outro PR, mas não precisa, pois já que é apenas uma definição de função, é tranquilo de fazer na mão e não tem problema depois rodar a migration por cima. Tanto que já defini a função (na versão sem Sobre o desempenho, eu acho que o caminho envolve melhorar a modelagem dos dados para não precisar usar a tabela de eventos. Isso seria facilmente alcançado com novos valores para Em um avanço maior, acho que deveríamos ter tabelas separadas para os balanços dos conteúdos e dos usuários, assim como TabCash também deveria ser separado. |
Acabei de fazer um teste aqui de quatro execuções consecutivas, primeiro com
Foram as únicas execuções que fiz no banco depois de ter ligado o computador (fora a atualização da função), então isso pode explicar o motivo da primeira execução de cada ter demorado significativamente mais.
Eu pensei nisso, mas não descobri como fazer com o
Sobre o Já no Outro ponto é o texto na UI que também precisa de uma melhoria (meu foco foi o desempenho na última iteração), mas isso já está sendo discutido no issue #1370.
Vou mudar no próximo commit 👍
Vamos supor que eu não consiga melhorar o tempo de execução das queries, e que essa sua proposta seja o caminho. Você acharia melhor implementá-la nesse PR ou deixar para outro, específico sobre isso? |
Muito estranho o impacto ser tão grande em
Pode colocar no returns: 'TABLE (total_balance BIGINT, total_credit BIGINT, total_debit BIGINT) ROWS 1',
Posso estar enganado, mas acho que compensa já ir para a mudança na tabela
Podemos colocar à prova a versão que já está no PR. Tendo os dados disponíveis no front, a forma de apresentação pode ser mudada facilmente, então podemos melhorar depois de acordo com o feedback dos usuários. 👍
Me parece que tanto faz. Mas, se for separar, acho que compensa ser um PR anterior a esse. Se achar que vale a pena ir por esse caminho, eu acho que já tenho uma boa ideia de como poderia ser feito, então posso tentar criar o primeiro PR. |
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
4f81846
to
0caeae8
Compare
Ok. Subi a alteração da frase conforme nosso último comentário no issue.
Pode implementar. Eu consegui testar algumas alterações na query do endpoint Coloquei duas opções de query abaixo. Você pode ver tanto as queries quanto o planejamento com o
A variação de tempo foi tão pequena que eu suspeito que as diferenças sejam apenas imprecisões, e não necessariamente que uma query ficou melhor que a outra. Minha análise principal foi pelo Além disso, o Por desencargo, fiz o teste também no cenário com 27 comentários, também sem alterações significativas:
PS: Eu percebi apenas depois dos testes acima que não precisava do |
Logo subo o PR 👍🤝 Acho que vou colocar também a versão atualizada de
Acho que a query original, com
É isso, vai ser muito pouca diferença, mas deve ser para pior. |
Ah, investigando mais a fundo, entendi. Acha melhor deixar com o |
Acho que é melhor deixar com PR #1624 aberto e a função |
Return TabCoins credits and debits when returning a content and when qualifying a content. The TabCoin initially earned by publishing the content is not considered when counting the `tabcoins_credit`. The `tabcoins_debit` is 0 or a negative number.
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
0caeae8
to
f3188a3
Compare
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
f3188a3
to
941ec0f
Compare
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
941ec0f
to
6ee2ab0
Compare
|
Decidi deixar padronizado para retornar o Criei um componente para exibir a Tooltip para garantir que o texto seja consistente independentemente de onde é exibido.
Retornei Aproveitei para tirar o Aproveitei para atualizar a descrição do PR. Pronto para revisão e potencial merge 👍 |
There was a problem hiding this comment.
Show, concordo com todas as últimas melhorias! 👍👍👍
Só veja se faz sentido minha sugestão no código ou se mantemos assim. Não é nada demais.
Testando esse PR em homologação eu percebi que alguns conteúdos estavam com dados estranhos, e pareciam ter 2 TabCoins iniciais (veja este exemplo).
Investigando isso, descobri que, em homologação, 111 conteúdos criados entre 01 e 03/07/2022 realmente ficaram com dois TabCoins iniciais, pois o script que o Filipe rodou para creditar os conteúdos antigos acabou duplicando os créditos nesse conteúdos. Isso deve ter ocorrido, provavelmente, porque nesse período existiam versões em homologação rodando em paralelo, algumas que creditavam e outras que ainda não creditavam TabCoins, então deve ter sido uma decisão entre duplicar alguns ao invés de deixar outros sem nenhum crédito inicial.
Então é isso, 111 conteúdos em homologação tem 2 TabCoins iniciais, mas em produção está tudo certo. 👍
Só que essa verificação acima me fez perceber que faltou um detalhe no UPDATE de balance_operations, pois as transações desfeitas devido ao banimento de usuários ficaram como content:tabcoin:initial, quando o correto era ser content:tabcoin:credit ou content:tabcoin:debit, e com sinais trocados, ou seja, para desfazer um crédito indevido, é criado um novo crédito com o valor negativo.
Já vou atualizar meu comentário no #1624 que descreve os scripts que devem ser executados para o UPDATE de balance_operations. Eu já rodei os scripts e fiz também o REINDEX. 👍
On hover, display TabCoins credits, debits and the percentage of credits in relation to the total.
Rafatcb
force-pushed
the
return-content-tabcoins-credits-and-debits
branch
from
6ee2ab0
to
be53167
Compare
aprendendofelipe
deleted the
return-content-tabcoins-credits-and-debits
branch
|
Em produção! 🚀🚀🚀 |
|
Fiz Rollback porque estava gerando erros de validação, o que impedia a exibição de alguns conteúdos. O problema é que alguns conteúdos de usuários banidos ficaram com créditos negativos, pois a reversão do crédito inicial ficou como |
|
Entendi. Existe algum conteúdo de usuário banido que ainda está na plataforma? Eu sei do problema de banir usuários com muitos conteúdos, que precisa apagar os conteúdos manualmente por causa do tempo da query, mas não sabia que conteúdos sem usuários ainda eram listados. |
Não é para ter nenhum. Está dando problema com os filhos dos conteúdos excluídos. |
|
As novas ações estão funcionando normalmente, o problema é apenas o |
Era isso sim, mas já está resolvido. Acabei de voltar a versão desse PR para produção. 🎉🎉🎉 Já atualizo de novo o #1624 (comment) |
Mudanças realizadas
Backend
Agora, duas novas propriedades são retornadas junto do conteúdo:
tabcoins_creditetabcoins_debit. Em consultas que retornavamtabcoinsde um conteúdo, essas novas propriedades também são retornadas (exemplo: ao qualificar um conteúdo).Endpoints afetados:
GET /api/v1/contentsPOST /api/v1/contentsGET /api/v1/contents/[username]GET /api/v1/contents/[username]/[slug]PATCH /api/v1/contents/[username]/[slug]GET /api/v1/contents/[username]/[slug]/childrenGET /api/v1/contents/[username]/[slug]/parentGET /api/v1/contents/[username]/[slug]/rootPOST /api/v1/contents/tabcoinsFrontend
A apresentação na UI é feita por meio de um Tooltip, tanto na lista de conteúdos quanto na página do conteúdo, para a publicação e para os comentários.
Resolve #1370
Tipo de mudança
Checklist: