cmd/trace-agent: default to YAML config by gbbr · Pull Request #435 · DataDog/datadog-trace-agent

gbbr · 2018-06-18T09:05:34Z

Small refactor of configuration code
Adds packages flags and osutil
Always default to Agent YAML config unless it's not there and we have a legacy config available.

LotharSee · 2018-06-19T09:20:49Z

-	// command-line arguments
-	// TODO: load from the .yaml automatically if there
-	flag.StringVar(&opts.configFile, "config", "/etc/dd-agent/datadog.conf", "Datadog Agent config file location")
-	flag.StringVar(&opts.legacyConfigFile, "ddconfig", "/etc/dd-agent/trace-agent.ini", "Deprecated extra configuration option.")


for context, the ddconfig config is from before the packaging with the Agent 5 (when the trace-agent was its own .deb) ; so it should be fair to remove it.

However, we will want to also remove the whole logic which goes with opts.legacyConfigFile.

LotharSee · 2018-06-19T21:03:53Z

 		conf:               conf,
 		dynConf:            dynConf,
 		ctx:                ctx,
-		die:                die,


I might have missed something, but how could a.die

datadog-trace-agent/cmd/trace-agent/agent.go

Line 315 in d087519

a.die("exceeded max memory (current=%d, max=%d)", wi.Mem.Alloc, int64(a.conf.MaxMemory))

work without this?

I don’t know! This is weird!

LotharSee · 2018-06-19T21:07:43Z

-func NewAgentConfig(conf *File, legacyConf *File, agentYaml *YamlAgentConfig) (*AgentConfig, error) {
-	c := NewDefaultAgentConfig()
-	var err error
+const legacyConfigFile = "/etc/dd-agent/datadog.conf"


Let's call it agent5 something of legacy, as it'd help understand it is still legit/relevant.

Windows need a different value. We could however avoid implementing this specific case by just checking how we package the Agent5 on Windows today and if it comes with the -config option.

LotharSee · 2018-06-19T21:09:03Z

-		err = mergeIniConfig(c, conf)
+// validate validates the given configuration, filling in any missing defaults
+// and merging any environment variables into it.
+func validate(c *AgentConfig) (*AgentConfig, error) {


Nit: I'd expect validate to only deal with errors/defaults and not the mergeEnv, as this is a real feature more than a simple validation.

Completely agree, this is confusing. It's just a temporary mid-refactor stage. Wasn't sure what to do with these yet.

gbbr · 2018-06-20T03:43:01Z

Thanks! All your comments are valid and I will address them. Note that I’ve marked the PR as WIP. In case it’s spamming you, you might want to unsubscribe until it’s ready for review. Otherwise, all comments are always welcome!

This change marks the INI configuration as deprecated and defaults the trace agent configuration to the YAML one. It also removes the previously deprecated `-ddconfig` flag (in version 5).

gbbr · 2018-06-20T10:19:02Z

@truthbk added you to check the following:

(config/agent.go).Load to confirm that the new settings are backwards compatible. They should be.
(config/agent.go).(*AgentConfig).acquireHostname for removing the Python shell code.

LotharSee · 2018-06-20T11:34:10Z

+	ErrMissingHostname = errors.New("failed to automatically set the hostname, you must specify it via configuration for or the DD_HOSTNAME env var")
+)
+
+const (


Should we move these to merge_env.go to make it more readable?

truthbk

Looks good to me, just added a note regarding one of the error messages, but feel free to ignore.

truthbk · 2018-06-20T21:33:54Z

 		}
-		c.HostName = hostname
+	default:
+		return cfg, errors.New("unrecognised file extension (need .yaml)")


I understand you're trying to deprecate .ini, .conf extensions, but this might be a little confusing since ini and conf extensions are actually supported.

I agree. Done.

* cmd/trace-agent: refactor config and default to yaml (with fallback) This change marks the INI configuration as deprecated and defaults the trace agent configuration to the YAML one. It also removes the previously deprecated `-ddconfig` flag (in version 5). * cmd/trace-agent: minimize init() functions and factor out shared flags. * fallback to legacy config if it's the only option * osutil, flags: add new packages to separate concerns. * config: improve config package. * move env constants to merge_env.go

gbbr force-pushed the gbbr/update-conf branch 4 times, most recently from 904e9da to 10b5d86 Compare June 18, 2018 11:15

gbbr requested a review from LotharSee June 18, 2018 11:24

gbbr changed the title ~~cmd/trace-agent: only default to INI file when it's the only option.~~ cmd/trace-agent: default to YAML config Jun 18, 2018

gbbr added this to the next milestone Jun 19, 2018

vlad-mh approved these changes Jun 19, 2018

View reviewed changes

LotharSee reviewed Jun 19, 2018

View reviewed changes

gbbr added the do-not-merge/WIP label Jun 19, 2018

gbbr force-pushed the gbbr/update-conf branch 7 times, most recently from 1618606 to eb82541 Compare June 19, 2018 19:59

LotharSee reviewed Jun 19, 2018

View reviewed changes

gbbr force-pushed the gbbr/update-conf branch 9 times, most recently from bbd0707 to 16d5061 Compare June 20, 2018 09:40

gbbr added 4 commits June 20, 2018 11:58

cmd/trace-agent: always default to YAML configuration file.

59cbb31

This change marks the INI configuration as deprecated and defaults the trace agent configuration to the YAML one. It also removes the previously deprecated `-ddconfig` flag (in version 5).

cmd/trace-agent: minimize init() functions and factor out shared flags.

73753de

WIP: fallback to legacy config if it's the only option

fd07002

osutil, flags: add new packages to separate concerns.

24130df

gbbr force-pushed the gbbr/update-conf branch 6 times, most recently from b369a13 to bf24147 Compare June 20, 2018 10:14

gbbr requested a review from truthbk June 20, 2018 10:16

gbbr added enhancement core and removed do-not-merge/WIP labels Jun 20, 2018

gbbr requested a review from LotharSee June 20, 2018 10:24

gbbr force-pushed the gbbr/update-conf branch from bf24147 to 3f5b53e Compare June 20, 2018 10:35

config: improve config package.

2133f28

gbbr force-pushed the gbbr/update-conf branch from 3f5b53e to 2133f28 Compare June 20, 2018 11:31

LotharSee reviewed Jun 20, 2018

View reviewed changes

move env constants to merge_env.go

b5e8357

gbbr force-pushed the gbbr/update-conf branch from 5c3e99b to b5e8357 Compare June 20, 2018 11:37

LotharSee approved these changes Jun 20, 2018

View reviewed changes

truthbk approved these changes Jun 20, 2018

View reviewed changes

gbbr and others added 2 commits June 21, 2018 08:37

update error message

e6c395e

Merge branch 'master' into gbbr/update-conf

ed868f2

gbbr merged commit c269811 into master Jun 21, 2018

gbbr deleted the gbbr/update-conf branch June 21, 2018 06:41

gbbr mentioned this pull request Jul 6, 2018

config: use Python environment as fallback to compute hostname #450

Merged

Uh oh!

Conversation

gbbr commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LotharSee Jun 19, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

gbbr commented Jun 20, 2018

Uh oh!

gbbr commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

truthbk 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gbbr commented Jun 18, 2018 •

edited

Loading

LotharSee Jun 19, 2018 •

edited

Loading