cmd/trace-agent: default to YAML config #435
Conversation
904e9da
to
10b5d86
Compare
// 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.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
1618606
to
eb82541
Compare
@@ -113,7 +113,6 @@ func NewAgent(ctx context.Context, conf *config.AgentConfig) *Agent { | |||
conf: conf, | |||
dynConf: dynConf, | |||
ctx: ctx, | |||
die: die, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t know! This is weird!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
config/agent.go
Outdated
func NewAgentConfig(conf *File, legacyConf *File, agentYaml *YamlAgentConfig) (*AgentConfig, error) { | ||
c := NewDefaultAgentConfig() | ||
var err error | ||
const legacyConfigFile = "/etc/dd-agent/datadog.conf" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
config/agent.go
Outdated
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree, this is confusing. It's just a temporary mid-refactor stage. Wasn't sure what to do with these yet.
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! |
bbd0707
to
16d5061
Compare
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).
b369a13
to
bf24147
Compare
@truthbk added you to check the following:
|
config/agent.go
Outdated
ErrMissingHostname = errors.New("failed to automatically set the hostname, you must specify it via configuration for or the DD_HOSTNAME env var") | ||
) | ||
|
||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move these to merge_env.go
to make it more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just added a note regarding one of the error messages, but feel free to ignore.
config/agent.go
Outdated
} | ||
c.HostName = hostname | ||
default: | ||
return cfg, errors.New("unrecognised file extension (need .yaml)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
flags
andosutil