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

Add logging agent in logger setup #3060

Merged
merged 8 commits into from
Mar 29, 2019
Merged

Add logging agent in logger setup #3060

merged 8 commits into from
Mar 29, 2019

Conversation

mfpierre
Copy link
Contributor

@mfpierre mfpierre commented Feb 19, 2019

What does this PR do?

Add logging agent in the logger setup.

Examples:

2019-02-19 14:46:01 UTC | CORE | ERROR | (pkg/logs/auditor/auditor.go:159 in recoverRegistry) | open /opt/datadog-agent/run/registry.json: no such file or directory
2019-02-19 14:49:21 UTC | DSD | INFO | (pkg/util/log/log.go:473 in func1) | Config will be read from env variables
2019-02-19 14:48:27 UTC | CLUSTER | ERROR | (pkg/util/kubernetes/apiserver/common/common.go:34 in GetMyNamespace) | There was an error fetching the namespace from the context, using default

Json:

{"agent":"core","time":"2019-02-19 15:24:43 UTC","level":"ERROR","file":"pkg/collector/scheduler.go","line":"184","func":"GetChecksFromConfigs","msg":"Unable to load the check: unable to load any check from config 'redisdb'"}

Motivation

To be merged once all agents are using the core log package as it will allow to remove the s6-format-filter

Fix #1599

Additional Notes

Anything else we should know when reviewing?

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #3060 into master will increase coverage by 1.57%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3060      +/-   ##
==========================================
+ Coverage   54.49%   56.07%   +1.57%     
==========================================
  Files         544      407     -137     
  Lines       38884    30241    -8643     
==========================================
- Hits        21190    16957    -4233     
+ Misses      16441    12430    -4011     
+ Partials     1253      854     -399
Flag Coverage Δ
#linux ?
#windows 56.07% <0%> (-0.01%) ⬇️
Impacted Files Coverage Δ
cmd/agent/app/app.go 100% <ø> (ø) ⬆️
cmd/agent/app/start.go 2.81% <0%> (-0.03%) ⬇️
cmd/agent/app/hostname.go 14.28% <0%> (ø) ⬆️
cmd/agent/app/jmx.go 16.66% <0%> (+0.53%) ⬆️
cmd/agent/app/diagnose.go 7.69% <0%> (-0.31%) ⬇️
cmd/agent/app/check.go 5.34% <0%> (+0.32%) ⬆️
pkg/config/log.go 3.24% <0%> (-0.11%) ⬇️
cmd/agent/app/flare.go 8% <0%> (ø) ⬆️
pkg/util/docker/util_common.go 0% <0%> (-100%) ⬇️
pkg/tagger/collectors/catalog.go 0% <0%> (-100%) ⬇️
... and 198 more

Copy link
Contributor

@gbbr gbbr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some thoughts.

@@ -20,8 +20,19 @@ import (
"github.com/cihub/seelog"
)

// LoggerName is a string prepended to all logs to indicate which component is logging
type LoggerName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of having a type here vs. plain string constants? The documentation itself is a hint. Why not just add a string argument to SetupLogger calling it prefix? It would be much clearer what it does (imho).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it would be preprended to all logs I feel having a dedicated type here helps enforcing that you know what you're doing when calling the function. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your motivation to underline the fact that this will show in every log. I'd be fine with that. Maybe we can change the documentation a bit and simply say:

// LoggerName specifies the name of an instantiated logger.

I feel a bit like saying "...is a string prepended to all logs..." is misleading because this declaration doesn't say anything about its usage. I'm thinking it'd be better suited to say that the name is prepended to all logs above SetupLogger.

But this is just a nit, I'm fine to keep it like this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel that this is misleading. This isn't the "name of an instantiated logger", it's just a "prefix that will be prepended to each logged line".

@mfpierre mfpierre force-pushed the mfpierre/logging-agent branch from 728a60b to 8b66988 Compare March 12, 2019 14:57
@mfpierre mfpierre marked this pull request as ready for review March 12, 2019 15:05
@mfpierre mfpierre requested a review from a team as a code owner March 12, 2019 15:05
@mfpierre mfpierre requested a review from a team March 12, 2019 15:05
@mfpierre mfpierre requested a review from a team as a code owner March 12, 2019 15:05
@mfpierre mfpierre force-pushed the mfpierre/logging-agent branch from da0a427 to 4b568de Compare March 29, 2019 12:23
@mfpierre mfpierre requested a review from a team as a code owner March 29, 2019 12:23
@mfpierre mfpierre force-pushed the mfpierre/logging-agent branch from 4b568de to 0d4e807 Compare March 29, 2019 12:25
@@ -20,8 +20,19 @@ import (
"github.com/cihub/seelog"
)

// LoggerName is a string prepended to all logs to indicate which component is logging
type LoggerName string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still feel that this is misleading. This isn't the "name of an instantiated logger", it's just a "prefix that will be prepended to each logged line".

---
enhancements:
- |
Change the logging format to include the name of the agent logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Change the logging format to include the name of the agent logging
Change the logging format to include the name of the logging agent.

Wasn't this included previously? We had [TRACE], [CORE], etc. What's the actual change? Or were some missing. Sorry if I missed something here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was present in the agent container yes due to the s6 filtering, I should probably rephrase 🤔

hush-hush
hush-hush previously approved these changes Mar 29, 2019
sunhay
sunhay previously approved these changes Mar 29, 2019
@mfpierre mfpierre merged commit 54ad0cb into master Mar 29, 2019
@mfpierre mfpierre deleted the mfpierre/logging-agent branch March 29, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants