Codecov Report

Merging #2678 into master will increase coverage by 0.34%.
The diff coverage is 56.66%.

@@            Coverage Diff             @@
##           master    #2678      +/-   ##
==========================================
+ Coverage   56.72%   57.06%   +0.34%     
==========================================
  Files         380      380              
  Lines       24122    24148      +26     
==========================================
+ Hits        13683    13780      +97     
+ Misses       9524     9477      -47     
+ Partials      915      891      -24

Just to clarify: the purpose of the legacy package it to allow the agent import command to read an Agent5 datadog.conf configuration and write an Agent 6 datadog.yaml config file from it.

Its purpose is not to allow the Agent 6 to read from a datadog.conf config at runtime.

So I don't see how this change would allow trace and process to support more config options in datadog.conf, I might be missing something though

@olivielpeau understood. I will try to explain.

The main idea is that we want to use the config package everywhere in all agents. Via this package we will access config values using their keys (which map to current yaml configuration, e.g. currently config.Datadog.Get("apm_config.enabled")). In process and trace we won't be able to do this nicely when packaged with Agent 5 because it uses the .ini config. Currently there is no way to load the .ini config into the global config (under the new key names) without the legacy package. However, the legacy package converts only partially, leaving some keys out. That's what this PR is for, to cover for those keys and to allow process and trace to seamlessly load the old configuration into the new one (mapped to the new keys).

The rest of the team also plans to remove all "setters" from the config, making it read only, so it won't be possible for us to read the ini and "write" the config. Even if it would be, I think that would be a bit too much duplication.

In the end, it should be possible for all agents to access keys from the config under the same name, regardless of where they were loaded from.

I hope this clarifies more. Do you still feel that this function belongs somewhere else? We could potentially move it into config under the name LoadLegacy but that would imply exporting more stuff from legacy and potentially breaking that API (maybe that's not a problem though).

Thanks for the explanation of the overall goal of the PR 👍

Just to explain where my thoughts come from:

1. setting aside the config refactoring in progress, the goal of the legacy package is to allow the agent import command to write an Agent 6 .yaml config that reflects the contents of an existing Agent 5 .ini config. So AFAIK any config option that the trace & process agents support in the Agent 5's .ini config should require changes to the legacy package anyway, since we want the agent import command to be able to read and convert all of these options.
2. we regard the Agent 5 as a software in "maintenance mode", which should not support any new config options unless they're helpful for the upgrade to Agent 6 (even though some exceptions were made for trace and process), and IMO at this point we should try to make the trace and process agents meet that goal as well. We can expect the Agent 5 to be deprecated relatively soon (we're still discussing the exact timeline with product), are we still adding regularly adding new config options to the trace and process agents that we want to support on an Agent 5 install? If we still do I think it should be time to stop soon.

So overall, these 2 points make me think that we should define the trace and process .ini options and how they map to Agent 6 options in the legacy package. We can expect this set of options to not change in the future given the maintenance status of Agent 5. The downside is that the .ini trace and process options would be defined in the Agent 6 repo instead of their respective repos, but that's OK to me since we can expect this to be short-lived.

Now, to address the use case of the trace + process agent being able to use the config package to load the Agent 6 config while still supporting the older Agent 5 config: using the legacy package directly sounds OK to me. I don't see an alternative that would not duplicate a lot of code and potentially introduce inconsistent behaviors.

Let me know what your thoughts on this are, and please discuss this with the agent guild as well.

One last thing:

In the end, it should be possible for all agents to access keys from the config under the same name, regardless of where they were loaded from.

For infra+logs, I agree as far as we're discussing the config options that are supported since Agent v6.
But we don't regard the Agent 5 .ini config format as simply a "different config source" given all the differences that were introduced between Agent v5 and v6.

Great! Thanks for taking the time to clarify. I like this plan. I was wondering myself why we continue adding options to the legacy config, given that you say it's in "maintenance mode" from your team's perspective.

In that case, I will make it a rule to stop adding any new options to the .ini config and instead open a new PR modifying FromAgentConfig to cover for all the current options in both process and trace.

I assume this would be the way moving forward and is what you're also saying.