Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autoscaler :: Adicionados logs mais detalhados #192

Merged
merged 20 commits into from Jan 8, 2020

Conversation

@rockerbacon
Copy link
Collaborator

rockerbacon commented Dec 26, 2019

Adicionados logs mais detalhados ao componente de decisão conforme discutido na issue #189. Foi mantido um entrada no log para cada parâmetro alterado onde cada entrada contém:

  • appid: id da aplicação alterada
  • event: identificação do que foi feito (CPU_SCALE ou MEM_SCALE)
  • previous_value: valor para o parâmetro antes da atuação do autoscaler
  • new_value: novo valor para o parâmetro conforme decidido pelo autoscaler

Mantive os eventos como CPU_SCALE e MEM_SCALE ao invés de CPU_SCALE_UP/DOWN e MEM_SCALE_UP/DOWN pois os campos previous_value e new_value já indicarão se houve aumento ou redução dos recursos alocados.

O namespace não foi incluído pois a API sendo utilizada para extrair dados do Asgard não traz essa informação.

@rockerbacon rockerbacon requested a review from daltonmatos as a code owner Dec 26, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 26, 2019

Codecov Report

Merging #192 into master will increase coverage by 0.15%.
The diff coverage is 88.88%.

Flag Coverage Δ
#typehint 47% <87.5%> (+0.1%) ⬆️
#unittest 97.34% <100%> (ø) ⬆️
Impacted Files Coverage Δ
asgard/workers/autoscaler/decision_events.py 100% <100%> (ø) ⬆️
...rd/workers/autoscaler/simple_decision_component.py 62.02% <81.81%> (+3.4%) ⬆️

logged_dict = mock_logger.info.call_args[0][0]

self.assertIn("appid", logged_dict, "did not log correct app id")

This comment has been minimized.

Copy link
@Dentxinho

Dentxinho Dec 26, 2019

Essas mensagens são em caso do assert não dar certo?

This comment has been minimized.

Copy link
@rockerbacon

rockerbacon Dec 27, 2019

Author Collaborator

Sim, elas são mostradas quando o assert falha

@daltonmatos

This comment has been minimized.

Copy link
Member

daltonmatos commented Dec 27, 2019

Mantive os eventos como CPU_SCALE e MEM_SCALE ao invés de CPU_SCALE_UP/DOWN e MEM_SCALE_UP/DOWN pois os campos previous_value e new_value já indicarão se houve aumento ou redução dos recursos alocados.

A ideia por trás do SCALE_UP/DOWN é que quando estivermos debugando para saber o que aconteceu com uma app não vamos precisar ficar fazendo conta para saber o que aconteceu, saca?

Um outro ponto relevante é que poderemos de forma fácil listar todos os eventos de SCALE_UP (ou DOWN) de uma (ou mais) apps. Não que não seja possível fazer essa listagem da forma com está hoje, mas da forma como está hoje mas mesmo com uma lista de registros na nossa frente ainda temos que "pensar" para saber o que aconteceu em cada evento.

Por iso voto em ter os eventos de UP e DOWN. A estrutura do log pode ser a mesma, com os valores anterior e posterior.

Enfim, esses são meus 0.02c. =D

Copy link
Member

daltonmatos left a comment

🎉 👍

asgard/workers/autoscaler/app.py Outdated Show resolved Hide resolved
@pabrrs

This comment has been minimized.

Copy link
Collaborator

pabrrs commented Dec 30, 2019

Mantive os eventos como CPU_SCALE e MEM_SCALE ao invés de CPU_SCALE_UP/DOWN e MEM_SCALE_UP/DOWN pois os campos previous_value e new_value já indicarão se houve aumento ou redução dos recursos alocados.

A ideia por trás do SCALE_UP/DOWN é que quando estivermos debugando para saber o que aconteceu com uma app não vamos precisar ficar fazendo conta para saber o que aconteceu, saca?

Um outro ponto relevante é que poderemos de forma fácil listar todos os eventos de SCALE_UP (ou DOWN) de uma (ou mais) apps. Não que não seja possível fazer essa listagem da forma com está hoje, mas da forma como está hoje mas mesmo com uma lista de registros na nossa frente ainda temos que "pensar" para saber o que aconteceu em cada evento.

Por iso voto em ter os eventos de UP e DOWN. A estrutura do log pode ser a mesma, com os valores anterior e posterior.

Enfim, esses são meus 0.02c. =D

Concordo com o @daltonmatos em adicionar os sufixos *_UP e *_DOWN nos eventos para facilitar na identificação.

Outra sugestão usar o campo event no logger como uma constante que facilite a reutilização por referência. Ex.:

# asgard/workers/autoscaler/events.py

CPU_SCALE_UP = "CPU_SCALE_UP"
CPU_SCALE_DOWN = "CPU_SCALE_DOWN"
MEM_SCALE_UP = "MEM_SCALE_UP"
MEM_SCALE_DOWN = "MEM_SCALE_DOWN"
# asgard/workers/autoscaler/simple_decision_component.py

from asgard.workers.autoscaler import events
...

self.logger.info(
    {
        "appid": app.id,
        "event": events.CPU_SCALE_UP,
        "previous_value": app.cpu_allocated,
        "new_value": decision.cpu,
    }
)

🚀

@rockerbacon rockerbacon requested a review from daltonmatos Jan 2, 2020
Copy link
Member

daltonmatos left a comment

👍 🎉

asgard/workers/autoscaler/decision_events.py Outdated Show resolved Hide resolved
rockerbacon added 2 commits Jan 3, 2020
@rockerbacon rockerbacon requested a review from daltonmatos Jan 3, 2020
@pabrrs
pabrrs approved these changes Jan 7, 2020
Copy link
Collaborator

pabrrs left a comment

Muito fowda 🎉

Dá uma olhada no coverage do typehint que ta com ziriguidum.

@daltonmatos daltonmatos mentioned this pull request Jan 8, 2020
6 of 6 tasks complete
@rockerbacon rockerbacon force-pushed the rockerbacon:feat/logs-autoscaler branch from d87669b to a317fcf Jan 8, 2020
Variáveis de Ambiente Obrigatórias
------------------------------------

- ``ASGARD_ASGARD_API_ADDRESS``: endereço da API do Asgard;

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jan 8, 2020

Collaborator

O nome da variável tem ASGARD duas vezes mesmo ?

This comment has been minimized.

Copy link
@rockerbacon

rockerbacon Jan 8, 2020

Author Collaborator

Sim, o projeto utiliza o prefixo ASGARD_ para algumas variáveis de ambiente. Internamente esse prefixo é removido e o código acessa a variável somente como ASGARD_API_ADDRESS

- ``asgard.autoscale.max_cpu_limit``: valor máximo que o autoscaler pode aplicar como parâmetro para CPU
- ``asgard.autoscale.min_cpu_limit``: valor mínimo que o autoscaler pode aplicar como parâmetro para CPU
- ``asgard.autoscale.max_mem_limit``: valor máximo que o autoscaler pode aplicar como parâmetro para memória
- ``asgard.autoscale.min_mem_limit``: valor mínimo que o autoscaler pode aplicar como parâmetro para memória
Comment on lines +65 to +68

This comment has been minimized.

Copy link
@pabrrs

pabrrs Jan 8, 2020

Collaborator

O que acha de usar namespace nas tag *.cpu e *.mem:

// CPU
asgard.autoscale.cpu.max_limit
asgard.autoscale.cpu.min_limit
// MEM
asgard.autoscale.mem.max_limit
asgard.autoscale.mem.min_limit
@daltonmatos daltonmatos merged commit a317fcf into b2wdigital:master Jan 8, 2020
7 checks passed
7 checks passed
ci/circleci: py368 Your tests passed on CircleCI!
Details
ci/circleci: py36x Your tests passed on CircleCI!
Details
ci/circleci: py37x Your tests passed on CircleCI!
Details
codecov/patch/typehint 87.5% of diff hit (target 46.89%)
Details
codecov/patch/unittest 100% of diff hit (target 97.34%)
Details
codecov/project/typehint 47% (+0.1%) compared to 831d537
Details
codecov/project/unittest 97.34% (+<.01%) compared to 831d537
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.