Display a warning in status when too many log tail #2265

achntrl · 2018-09-03T15:22:57Z

What does this PR do?

This PR displays a warning in the agent status when the user is trying to tail more files than the logs_config.open_files_limit is allowing

Motivation

For now, this happens almost silently, the only way to find out that not all files are tailed is to read the logs of agent. This PR makes the problem more visible

tmichelet · 2018-09-03T15:47:26Z

@achntrl thanks for working on this! Someone from the team will review the code shortly, in the meantime, would you mind adding some screenshots of the agent status, to show how it displays? It helps the reviewer to picture how it would look like :)

achntrl · 2018-09-03T15:54:10Z

Terminal status page :

Html status page :

achntrl · 2018-09-03T16:58:55Z

I found a small bug: If you have 2 folders with > 100 files, then you delete files and you go under 100 files then you add more than 100 files in the first folder and some files in the second folder, the counter for the second folder doesn't get updated

It seems very specific but it's because of this loop

It shouldn't be too hard to fix, I'll push a fix tomorrow (and hopefully a test!)

tmichelet · 2018-09-04T06:26:39Z

This is great, adding the number of tailed files per wildcard tailer is more than I hoped we'd do, 👏 .

tmichelet · 2018-09-04T06:27:37Z

pkg/logs/status/warnings.go

+				messages = append(
+					messages,
+					fmt.Sprintf(
+						"The default limit on the maximum number of files in use (%d) has been reached. Increase this limit (thanks to the attribute logs_config.open_files_limit in datadog.yaml) or decrease the number of tailed file.",


We can still reach the limit if we override the files limit -> it's not always the default limit that has been reached

Yes, the number displayed is the actual configured number, I will update the wording

tmichelet · 2018-09-04T06:29:44Z

pkg/logs/status/status.go

+		if count == len(inputs) {
+			return fmt.Sprintf("This path tails %d files", count)
+		}
+		return fmt.Sprintf("This path tries to tail %d files but only %d files are tailed", count, len(inputs))


General nit: I'd avoid tries to and but only in agent statuses, as those tend to be blurry words.
What about something like %d files tailed out of %d files matching that we could use for both cases?

This path .. -> %s ..." %{path}

ajacquemot

Thanks for working on this feature, the main logic is here but I'd like to raise any kind of warning from any kind of component, could you address this problem too please ?

ajacquemot · 2018-09-04T07:41:57Z

pkg/logs/status/status.go

@@ -111,3 +119,17 @@ func toDictionary(c *config.LogsConfig) map[string]interface{} {
 	}
 	return dictionary
 }
+
+func generateOpenFilesLimitReachedWarning(c *config.LogsConfig, inputs []string) string {


This method should not be here, the status should be agnostic of the type of warning.

ajacquemot · 2018-09-04T07:43:01Z

pkg/logs/status/status.go

+				Configuration:  toDictionary(source.Config),
+				Status:         status,
+				Inputs:         inputs,
+				WarningMessage: generateOpenFilesLimitReachedWarning(source.Config, inputs),


I would expect something like:

WarningMessage: source.Warning.Render(),

ajacquemot · 2018-09-04T07:45:56Z

pkg/logs/status/status.go

+		if count == len(inputs) {
+			return fmt.Sprintf("This path tails %d files", count)
+		}
+		return fmt.Sprintf("This path tries to tail %d files but only %d files are tailed", count, len(inputs))


This path .. -> %s ..." %{path}

ajacquemot · 2018-09-04T08:06:00Z

pkg/logs/input/file/file_provider.go

@@ -34,6 +35,7 @@ func NewFile(path string, source *config.LogSource) *File {
 type Provider struct {
 	filesLimit      int
 	shouldLogErrors bool
+	reachedLimit    bool


this is not used

ajacquemot · 2018-09-04T08:07:47Z

pkg/logs/input/file/file_provider.go

@@ -71,9 +73,11 @@ func (p *Provider) FilesToTail(sources []*config.LogSource) []*File {

 	if len(filesToTail) == p.filesLimit {
 		log.Warn("Reached the limit on the maximum number of files in use: ", p.filesLimit)
+		status.Warnings.RaisedWarnings[status.OpenFilesLimitReachedType] = true


I think if the interface of Warnings was status.Warnings.Raise(key string, message string), we could add any kind of message

ajacquemot · 2018-09-04T08:09:25Z

pkg/logs/input/file/file_provider.go

 		return filesToTail
 	}

+	status.Warnings.RaisedWarnings[status.OpenFilesLimitReachedType] = false


There it would be status.Warnings.Solve(key string)

ajacquemot · 2018-09-04T08:10:33Z

pkg/logs/status/status.go

+	Configuration  map[string]interface{} `json:"configuration"`
+	Status         string                 `json:"status"`
+	Inputs         []string               `json:"inputs"`
+	WarningMessage string                 `json:"warning"`


I think you should have a Warning struct here that is renderable.

ajacquemot · 2018-09-04T08:11:37Z

pkg/logs/status/warnings.go

+
+// Warning types
+const (
+	OpenFilesLimitReachedType = "open_files_limit_reached"


I don't think Warnings should be aware of the kind of warning.

ajacquemot · 2018-09-04T08:13:12Z

pkg/status/dist/templates/logsagent.tmpl

@@ -4,6 +4,11 @@ NOTE: Changes made to this template should be reflected on the following templat
 */}}==========
 Logs Agent
 ==========
+{{- if .warnings }}


I think we should add this block after line 9 because can't raise warnings if the agent is not running

ajacquemot · 2018-09-04T08:13:27Z

pkg/status/dist/templates/logsagent.tmpl

@@ -4,6 +4,11 @@ NOTE: Changes made to this template should be reflected on the following templat
 */}}==========
 Logs Agent
 ==========
+{{- if .warnings }}


I think you want {{ if .warnings }}

ajacquemot · 2018-09-04T20:28:56Z

pkg/logs/warning/warning.go

+
+// Remove marks a RaisedWarning as solved
+func Remove(key string) {
+	delete(w.raised, key)


This method is not safe, you should check if the entry exists first

If the entry does not exists, delete is a noop see

ajacquemot · 2018-09-04T20:34:41Z

pkg/logs/warning/warning.go

+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2018 Datadog, Inc.
+
+package warning


This package needs to be unit-tested, especially the operations on the map since multiple clients on different threads could call Raise and Get roughly at the same time.
For more details, you can check https://stackoverflow.com/questions/36167200/how-safe-are-golang-maps-for-concurrent-read-write-operations

ajacquemot · 2018-09-04T20:36:59Z

This looks great and fully reusable, I left two comments but we're heading to the right direction.
Thanks

ajacquemot · 2018-09-05T07:44:51Z

pkg/logs/warning/warning.go

+// This product includes software developed at Datadog (https://www.datadoghq.com/).
+// Copyright 2018 Datadog, Inc.
+
+package warning


Also I think we should move this package/file to ./pkg/logs/status

ajacquemot · 2018-09-05T09:18:50Z

pkg/logs/status/warning_test.go

+	return "foo"
+}
+
+func TestRaise(t *testing.T) {


I'd rather not test the inner map of the warnings struct but just that Raise followed by a Get returns what I expect

ajacquemot · 2018-09-05T09:19:19Z

pkg/logs/status/warning_test.go

+	assert.Equal(t, []Warning{concreteWarning1, concreteWarning2}, GetWarnings())
+}
+
+func TestRemove(t *testing.T) {


Same, I'd rather do something like Raise, Get, Remove, Get

ajacquemot

LGTM

mnshdw

I think the code would be much simpler if we concentrated on having the possibility to display an arbitrary list of messages. It would be much more re-usable and we still can have the file provider use it to register a warning when we are tailing too many files.

mnshdw · 2018-09-05T11:18:57Z

pkg/logs/status/status.go

-	Integrations []Integration `json:"integrations"`
+	IsRunning       bool          `json:"is_running"`
+	Integrations    []Integration `json:"integrations"`
+	WarningMessages []string      `json:"warnings"`


I think this could have been simply generalized into Messages.
If we need a severity we can always add that later.

mnshdw · 2018-09-05T11:21:50Z

pkg/logs/status/warning.go

+var w = newWarnings()
+
+// Warning is a generic interface that generate warning messages
+type Warning interface {


It's just a string so I wonder if the abstraction is really necessary.

mnshdw · 2018-09-05T11:24:48Z

pkg/logs/config/source.go

+	Config   *LogsConfig
+	Status   *LogStatus
+	inputs   map[string]bool
+	Overview string


I think Messages would be more appropriate here as well as it doesn't convey additional semantics that aren't necessarily true here.

ajacquemot · 2018-09-06T14:49:10Z

pkg/logs/status/status.go

@@ -61,6 +61,7 @@ func Get() Status {
 	}
 	// Convert to json
 	var integrations []Integration
+	warningsDeduplicator := make(map[string]bool)


nits: make(map[string]bool) -> make(map[string]struct{})

ajacquemot · 2018-09-06T14:49:49Z

pkg/logs/status/status.go

 			})
+
+			for _, warning := range source.Messages.GetWarnings() {
+				warningsDeduplicator[warning] = true


warningsDeduplicator[warning] = true -> warningsDeduplicator[warning] = struct{}{}

ajacquemot · 2018-09-06T14:51:09Z

pkg/logs/status/status.go

-	warningMessages := make([]string, len(warnings))
-	for i, w := range warnings {
-		warningMessages[i] = w.Render()
+	warnings := make([]string, 0)


warnings := make([]string, 0) -> var warnings []string

ajacquemot · 2018-09-06T14:53:09Z

pkg/logs/status/status.go

-		WarningMessages: warningMessages,
+		IsRunning:    true,
+		Integrations: integrations,
+		Messages:     warnings,


I think this is a bit misleading, I am not sure the messages that we want to report at the very beginning of the logs status are always related to logs-sources.
We could for example report to the user that he is using a deprecated attribute and the he should use the new one.

ajacquemot · 2018-09-06T14:54:56Z

pkg/logs/status/status.go

@@ -61,6 +61,7 @@ func Get() Status {
 	}
 	// Convert to json
 	var integrations []Integration
+	warningsDeduplicator := make(map[string]bool)


If a warning/message was not tight to a source, you'd need to do any deduplicate here.

Yes, but the alternative would have been to compute the warning at the status create but this would require computer to identify LogSource for file an count the inputs

I found it messier than just deduplicating warning messages (which is quite a small operation)

When the user tails too many log files, some files are not tailed but the status doesn't report any problem (only in the logs of the agent)

ajacquemot · 2018-09-07T08:31:58Z

pkg/logs/config/messages.go

+
+// AddWarning create a warning
+func (m *Messages) AddWarning(key string, warning string) {
+	m.AddMessage("warning_"+key, warning)


Why are you adding a prefix here ? If it's just for filtering out purpose, I'd rather avoid doing that because a string compare is not efficient.
You could create a structure:

type Message struct { Value String IsWarning bool }

And use IsWarning for filtering out

It's to avoid collision between keys and to reuse the same methods so most warning methods are just calling the message one with the prefix warning_

remicalixte · 2018-09-18T15:21:45Z

pkg/logs/config/messages.go

+	"sync"
+)
+
+// Messages holds a message message


Is "message" twice normal?

remicalixte · 2018-09-18T15:22:38Z

pkg/logs/config/source.go

-import "sync"
+import (
+	"sync"
+)

 // LogSource holds a reference to and integration name and a log configuration, and allows to track errors and


Typo: to "an" integration

achntrl requested review from a team as code owners September 3, 2018 15:22

achntrl added [deprecated] team/logs do-not-merge/WIP labels Sep 3, 2018

achntrl force-pushed the achntrl/tailed-logs-warning-in-status branch from 71550a3 to c3fd1d2 Compare September 3, 2018 15:43

achntrl removed the do-not-merge/WIP label Sep 3, 2018

tmichelet reviewed Sep 4, 2018

View reviewed changes

ajacquemot requested changes Sep 4, 2018

View reviewed changes

achntrl force-pushed the achntrl/tailed-logs-warning-in-status branch 2 times, most recently from 905e242 to 8cf832b Compare September 4, 2018 16:16

ajacquemot reviewed Sep 4, 2018

View reviewed changes

ajacquemot reviewed Sep 5, 2018

View reviewed changes

achntrl force-pushed the achntrl/tailed-logs-warning-in-status branch from a1de789 to ec760b4 Compare September 5, 2018 08:38

ajacquemot added this to the 6.6.0 milestone Sep 5, 2018

ajacquemot reviewed Sep 5, 2018

View reviewed changes

ajacquemot previously approved these changes Sep 5, 2018

View reviewed changes

mnshdw requested changes Sep 5, 2018

View reviewed changes

olivielpeau requested a review from remicalixte September 5, 2018 12:45

achntrl dismissed ajacquemot’s stale review via babfc63 September 6, 2018 14:25

ajacquemot reviewed Sep 6, 2018

View reviewed changes

achntrl added 8 commits September 7, 2018 09:20

Display a warning in status when too many log tail

d0cb707

When the user tails too many log files, some files are not tailed but the status doesn't report any problem (only in the logs of the agent)

Make warning threadsafe

82eb928

Add tests to logs/warning module

759bef6

Move logs/warning to logs/status package

524885e

Update test to use only public methods

6b12806

Transform Warning into more generic Messages linked to sources

8141ca6

Update function comments

6a4021e

Nits: update code style

2481ddc

achntrl force-pushed the achntrl/tailed-logs-warning-in-status branch from c9ba6dd to 2481ddc Compare September 7, 2018 07:31

ajacquemot reviewed Sep 7, 2018

View reviewed changes

mnshdw previously approved these changes Sep 7, 2018

View reviewed changes

remicalixte requested changes Sep 18, 2018

View reviewed changes

Improve comments

49a43d8

achntrl dismissed mnshdw’s stale review via 49a43d8 September 18, 2018 20:37

remicalixte approved these changes Sep 18, 2018

View reviewed changes

achntrl merged commit debcc81 into master Sep 18, 2018

achntrl deleted the achntrl/tailed-logs-warning-in-status branch September 18, 2018 21:06

ajacquemot added the component/logs label Oct 12, 2018

Display a warning in status when too many log tail #2265

Display a warning in status when too many log tail #2265

Conversation

achntrl commented Sep 3, 2018

What does this PR do?

Motivation

tmichelet commented Sep 3, 2018

achntrl commented Sep 3, 2018

achntrl commented Sep 3, 2018

tmichelet commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajacquemot left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajacquemot commented Sep 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajacquemot left a comment

Choose a reason for hiding this comment

mnshdw left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnshdw Sep 5, 2018 • edited

Choose a reason for hiding this comment

ajacquemot Sep 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajacquemot left a comment •

edited

mnshdw left a comment •

edited

mnshdw Sep 5, 2018 •

edited

ajacquemot Sep 6, 2018 •

edited