PMM-5680 store logs by qwest812 · Pull Request #342 · percona/pmm-agent

qwest812 · 2022-03-23T22:01:52Z

PMM-5680 store agents logs pmm-admin#197

YaroslavPodorvanov · 2022-04-05T08:48:01Z

storelogs/main.go

+	dt := time.Now()
+	log = dt.Format("01-02-2006 15:04:05") + " " + l.entry.Level.String() + ": " + log
+	for _, v := range l.entry.Data {
+		log = log + fmt.Sprintf("    %v", v)


1 solution make slice of strings and then join
2 solution by any buffer
in log = log + fmt.Sprintf(" %v", v) too many garbage https://gosamples.dev/concatenate-strings/

changed to buffer

YaroslavPodorvanov · 2022-04-12T10:13:25Z

agentlocal/agent_local.go

 	reloadCloseOnce sync.Once
 }

+func (s *Server) mustEmbedUnimplementedAgentLocalServer() {


it's wrong, possible solution remove this requirement by some protoc --arg or by embeding [agentlocalpb|agentpb].UnimplementedAgentLocalServer

YaroslavPodorvanov · 2022-04-12T10:16:34Z

storelogs/main.go

+
+type LogsStore struct {
+	log   *ring.Ring
+	Entry *logrus.Entry


entry *logrus.Entry

I need use it here newProcessLogger(sl.Entry, keepLogLines, redactWords),

YaroslavPodorvanov

mustEmbed need delete

# Conflicts: # go.mod # go.sum

github-actions · 2022-05-10T13:34:24Z

agentlocal/agent_local.go

 }

+//// AgentLogs contains information about Agent logs.
+//type AgentLogs struct {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
commentFormatting: put a space between // and comment text (gocritic)

github-actions · 2022-05-10T13:34:24Z

commands/run.go

 	// handle termination signals
 	signals := make(chan os.Signal, 1)
 	signal.Notify(signals, unix.SIGTERM, unix.SIGINT)
+	ringLog := storelogs.New(500)


🚫 [golangci-lint] _{reported by reviewdog 🐶}
mnd: Magic number: 500, in detected (gomnd)

BupycHuk · 2022-05-10T15:28:27Z

agents/supervisor/supervisor.go

+	var res map[string][]string
+
+	for id, agent := range s.agentProcesses {
+		res[fmt.Sprintf("%s %s", id, agent.requestedState.Type.String())] = agent.logs.GetLogs()


let's put type to the beginning and let's remove /agent_id/ from id.

github-actions · 2022-05-10T16:00:40Z

commands/run.go

 	"github.com/percona/pmm-agent/versioner"
 )

+const COUNT_SERVER_LOGS = 500


🚫 [golangci-lint] _{reported by reviewdog 🐶}
don't use ALL_CAPS in Go names; use CamelCase (golint)

github-actions · 2022-05-10T16:00:40Z

agents/supervisor/supervisor.go

 const (
-	type_TEST_SLEEP inventorypb.AgentType = 998 // process
-	type_TEST_NOOP  inventorypb.AgentType = 999 // built-in
+	type_TEST_SLEEP  inventorypb.AgentType = 998 // process


🚫 [golangci-lint] _{reported by reviewdog 🐶}
var-naming: don't use underscores in Go names; const type_TEST_SLEEP should be typeTESTSLEEP (revive)

github-actions · 2022-05-10T16:00:40Z

agents/supervisor/supervisor.go

-	type_TEST_SLEEP inventorypb.AgentType = 998 // process
-	type_TEST_NOOP  inventorypb.AgentType = 999 // built-in
+	type_TEST_SLEEP  inventorypb.AgentType = 998 // process
+	type_TEST_NOOP   inventorypb.AgentType = 999 // built-in


🚫 [golangci-lint] _{reported by reviewdog 🐶}
var-naming: don't use underscores in Go names; const type_TEST_NOOP should be typeTESTNOOP (revive)

github-actions · 2022-05-10T16:00:40Z

agents/supervisor/supervisor.go

-	type_TEST_NOOP  inventorypb.AgentType = 999 // built-in
+	type_TEST_SLEEP  inventorypb.AgentType = 998 // process
+	type_TEST_NOOP   inventorypb.AgentType = 999 // built-in
+	COUNT_AGENT_LOGS                       = 10


🚫 [golangci-lint] _{reported by reviewdog 🐶}
var-naming: don't use ALL_CAPS in Go names; use CamelCase (revive)

let's use camelCase.
In the cases above snake_case is used because they are mocks for enum and it's better to move them to supervisor_test.go

github-actions · 2022-05-10T21:16:31Z

agents/supervisor/supervisor.go

+	res := make(map[string][]string)
+
+	for id, agent := range s.agentProcesses {
+		newId := strings.ReplaceAll(id, "/agent_id/", "")


🚫 [golangci-lint] _{reported by reviewdog 🐶}
var newId should be newID (golint)

github-actions · 2022-05-10T21:16:31Z

agents/supervisor/supervisor.go

+	}
+
+	for id, agent := range s.builtinAgents {
+		newId := strings.ReplaceAll(id, "/agent_id/", "")


🚫 [golangci-lint] _{reported by reviewdog 🐶}
var newId should be newID (golint)

BupycHuk

looks good to me, just a few minor comments

BupycHuk · 2022-05-11T11:24:05Z

agentlocal/agent_local.go


 // NewServer creates new server.
-//
+//`


let's revert

BupycHuk · 2022-05-11T11:25:36Z

agentlocal/agent_local.go

 	mux.Handle("/debug/", http.DefaultServeMux)
 	mux.Handle("/debug", debugPageHandler)
 	mux.Handle("/", proxyMux)
+	mux.HandleFunc("/logs.zip", func(w http.ResponseWriter, r *http.Request) {


can we add a test for logs.zip?

BupycHuk · 2022-05-11T11:26:15Z

agents/supervisor/supervisor.go

-	type_TEST_NOOP  inventorypb.AgentType = 999 // built-in
+	type_TEST_SLEEP  inventorypb.AgentType = 998 // process
+	type_TEST_NOOP   inventorypb.AgentType = 999 // built-in
+	COUNT_AGENT_LOGS                       = 10


let's increase the value

BupycHuk · 2022-05-11T11:31:07Z

agents/supervisor/supervisor.go

-	type_TEST_NOOP  inventorypb.AgentType = 999 // built-in
+	type_TEST_SLEEP  inventorypb.AgentType = 998 // process
+	type_TEST_NOOP   inventorypb.AgentType = 999 // built-in
+	COUNT_AGENT_LOGS                       = 10


let's use camelCase.
In the cases above snake_case is used because they are mocks for enum and it's better to move them to supervisor_test.go

# Conflicts: # agents/supervisor/supervisor.go

github-actions · 2022-05-20T07:24:35Z

agentlocal/agent_local.go

 	_ agentlocalpb.AgentLocalServer = (*Server)(nil)
 )
+
+func (s *Server) Zip(w http.ResponseWriter, r *http.Request) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
exported method Server.Zip should have comment or be unexported (golint)

github-actions · 2022-05-20T07:24:35Z

agentlocal/agent_local_test.go

 	})
 }
+
+func TestGetZipFile(t *testing.T) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
Function TestGetZipFile missing the call to method parallel (paralleltest)

github-actions · 2022-05-20T07:24:35Z

agentlocal/agent_local_test.go

 }
+
+func TestGetZipFile(t *testing.T) {
+	setup := func(t *testing.T) ([]*agentlocalpb.AgentInfo, *mockSupervisor, *mockClient, *config.Config) {


🚫 [golangci-lint] _{reported by reviewdog 🐶}
test helper function should start from t.Helper() (thelper)

github-actions · 2022-05-20T07:24:35Z

agentlocal/agent_local_test.go

+	for _, serverLog := range s.ringLogs.GetLogs() {
+		_, err := b.WriteString(serverLog)
+		if err != nil {
+			return nil, err


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error returned from external package is unwrapped: sig: func (*bytes.Buffer).WriteString(s string) (n int, err error) (wrapcheck)

github-actions · 2022-05-20T07:24:36Z

agentlocal/agent_local_test.go

+		for _, l := range logs {
+			_, err := b.WriteString(l + "\n")
+			if err != nil {
+				return nil, err


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error returned from external package is unwrapped: sig: func (*bytes.Buffer).WriteString(s string) (n int, err error) (wrapcheck)

github-actions · 2022-05-20T07:24:36Z

agentlocal/agent_local_test.go

+	}
+	err := writer.Close()
+	if err != nil {
+		return nil, err


🚫 [golangci-lint] _{reported by reviewdog 🐶}
error returned from external package is unwrapped: sig: func (*archive/zip.Writer).Close() error (wrapcheck)

ritbl · 2022-05-22T13:09:00Z

agentlocal/agent_local_test.go

+		b, err := ioutil.ReadAll(rec.Body)
+		require.NoError(t, err)
+
+		expectedFile, err := generateTestZip(s)


You are generating zip file that you will later compare with, but what if it fails? The output will be not readable.
I think it would be better to unzip received file, and verify content.
Here is example of equality check failing:

The failure message is impossible to understand, because it is a diff between two binary files

ritbl

please see my comment

BupycHuk · 2022-06-01T07:02:55Z

agentlocal/agent_local.go

-//
+


let's revert it.

BupycHuk · 2022-06-01T07:05:54Z

agentlocal/agent_local.go

+			log.Fatal(err)
+		}
+	}
+	addData(writer, "server.txt", b.Bytes())


I think it's better to rename it to pmm-agent.txt or http-server if it contains only http server logs.

Suggested change

addData(writer, "server.txt", b.Bytes())

addData(writer, "http-server.txt", b.Bytes())

BupycHuk · 2022-06-01T07:14:46Z

storelogs/storelogs.go

+func (l *LogsStore) Write(b []byte) (n int, err error) {
+	l.m.Lock()
+	l.log.Value = string(b)
+	l.m.Unlock()


let's unlock it one line below.

qwest812 added 9 commits March 20, 2022 23:32

Pmm-5680 store logs

167cbf3

Pmm-5680 store logs

29764cc

Merge branch 'main' into PMM-5680-store-logs

c8d2f68

Pmm-5680 store logs

b673eaa

Pmm-5680 store logs

9eda60e

Pmm-5680 store logs

a6faa0b

Pmm-5680 store logs

6f61509

Pmm-5680 store logs

20a3ee2

Pmm-5680 save logs process Done

229845d

This was referenced Apr 2, 2022

PMM-5680 store agents logs percona/pmm-admin#197

Open

PMM-5680 store logs Percona-Lab/pmm-submodules#2425

Closed

PMM-5680 add log to struct AgentInfo percona/pmm#857

Closed

YaroslavPodorvanov reviewed Apr 5, 2022

View reviewed changes

Pmm-5680 save logs process

9161345

YaroslavPodorvanov suggested changes Apr 12, 2022

View reviewed changes

qwest812 requested a review from YaroslavPodorvanov April 12, 2022 10:28

YaroslavPodorvanov suggested changes Apr 12, 2022

View reviewed changes

qwest812 mentioned this pull request Apr 13, 2022

PMM-5680 add log to struct AgentInfo percona/pmm#866

Closed

2 tasks

qwest812 added 12 commits April 14, 2022 15:40

Pmm-5680 change dependency

aa77af2

Merge branch 'main' into PMM-5680-store-logs

c6281e4

# Conflicts: # go.mod # go.sum

Pmm-5680 update dependency

5163ca9

Pmm-5680 add mutex

e35cbc4

Pmm-5680 fix tests

89a98da

Pmm-5680 fix tests

989bb4f

Pmm-5680 fix tests

9169b92

Pmm-5680 fix tests

8562fbc

Pmm-5680 change process_child.go

f0b2d6a

Pmm-5680 fix process_child.go

7a4863a

Pmm-5680 fix mysql_show_table_status_action_test.go

c20a64c

Pmm-5680 gen

f2d3a40

qwest812 added 3 commits May 10, 2022 11:17

Pmm-5680 store serve logs

858e68c

Pmm-5680 fix test

611c10e

Pmm-5680 remove struct AgentLogs

49cc0e6

github-actions bot reviewed May 10, 2022

View reviewed changes

BupycHuk reviewed May 10, 2022

View reviewed changes

qwest812 added 2 commits May 10, 2022 18:46

Pmm-5680 remove id from map key

60a0db6

Pmm-5680 set const number logs

ed5a7ce

github-actions bot reviewed May 10, 2022

View reviewed changes

Pmm-5680 change function AgentsLogs

9d688bb

github-actions bot reviewed May 10, 2022

View reviewed changes

qwest812 requested a review from BupycHuk May 11, 2022 08:54

qwest812 added 2 commits May 11, 2022 12:04

Pmm-5680 remove comment

545883a

Pmm-5680 fix linn err

96f10b0

BupycHuk approved these changes May 11, 2022

View reviewed changes

qwest812 added 6 commits May 11, 2022 14:49

Pmm-5680 add small changes

9880641

Pmm-5680 create test

60e10a4

Pmm-5680 фвв еуые яшз ашду

a0b10af

Merge branch 'main' into PMM-5680-store-logs

729bc50

# Conflicts: # agents/supervisor/supervisor.go

Pmm-5680 make gen

ecbac3d

Pmm-5680 fix test

592965b

github-actions bot reviewed May 20, 2022

View reviewed changes

ritbl reviewed May 22, 2022

View reviewed changes

ritbl suggested changes May 22, 2022

View reviewed changes

qwest812 added 3 commits May 25, 2022 23:53

Pmm-5680 fix test

f379d59

Pmm-5680 fix test

29028f2

Pmm-5680 fix test

f43cd91

qwest812 requested a review from BupycHuk May 30, 2022 19:10

BupycHuk approved these changes Jun 1, 2022

View reviewed changes

pmm-5680 little changes

796c46d

qwest812 requested a review from ritbl June 1, 2022 12:09

	addData(writer, "server.txt", b.Bytes())
	addData(writer, "http-server.txt", b.Bytes())

Conversation

qwest812 commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qwest812 Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YaroslavPodorvanov left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 10, 2022

Choose a reason for hiding this comment

Uh oh!

BupycHuk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot May 20, 2022

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ritbl left a comment

Choose a reason for hiding this comment

qwest812 commented Mar 23, 2022 •

edited

Loading

qwest812 Apr 12, 2022 •

edited

Loading