Remove overwritepropertiesconverter and search for a solution for --set

[bogdandrutu]

The main motivation is because the --set flag and implicitly the overwritepropertiesconverter uses dot (.) as the key separator which makes it impossible to configure maps that contain keys with possible dots, e.g. in service::telemetry::resource where attribute keys contain dots. This was one of the main reason we changed confmap to use "::".

Updated: Also the flag has a limitation for array values, see the flag comment:

Array config properties are overridden and maps are joined, note that only a single (first) array property can be set e.g. --set=processors.attributes.actions.key=some_key.

Because of this issue, I suggest that we stop spreading this "not ideal" usage and:

1. Remove overwritepropertiesconverter (for sure go via the process of deprecation) and transform the value passed to --set into a calls like --config=yaml:....
   • For users this will be no-op.
   • For the distributions using the builder this will be no-op.
   • For the distributions not using the builder they will have to duplicate that small code that translates.
2. For the --set flag, we should consider marking this as deprecated and keep it for some months, with the deprecation message being clear on how to transform that call into a --config=yaml:... equivalent.

[dmitryax]

I think using --set processors.batch.timeout=2s is much more convenient than --config=yaml:processors::batch::timeout: 2s. I think --set argument is pretty intuitive and used by other applications. For example, helm uses the same approach where dots in from yaml keys can be escaped like --set 'app.podLabels.app\.kubernetes\.io/part-of=security'. Array values can be overridden there with --set ingress.hosts[0]=auth1.

Is this reasonable to do the same in collector and keep . separator just for --set argument?

Or maybe we have something like --configval processors::batch::timeout=2s?

cc @open-telemetry/collector-approvers

[bogdandrutu]

To summarize my understanding, you don't disagree with the 1. point of removing the overwritepropertiesconverter, but you will want a solution for the users which is easier to use, and believe that the current non "--set" solution is hard to use.

I do agree that --config=yaml:processors::batch::timeout: 2s may not be as convenient (looking at exact difference is not that bad "--set=" is replaced by "--config=yaml:", "." with "::", and "=" with ":", but let's assume it is that bad), but is way more powerful:

• Supports setting arrays --config=yaml:processors::batch::timeout: [1, 2, 3].
• Supports setting maps --config=yaml:processors::batch::timeout: {"foo" : "bar" }.

An option is to indeed not deprecate "--set" and expand it, which we do with this PR anyway (so just revert the deprecation notice for the flag), because we add support for the value to be array/map/etc. Then we need to fix the escaping.

Personally I think that will be very confusing to the users to have different separators, since in the main config you cannot write processors.batch.timeout=2s but you can write processors::batch::timeout: 2s and will do the right thing.

[bogdandrutu]

From @dmitryax the link to that "--set" flag is here https://helm.sh/docs/intro/using_helm/#the-format-and-limitations-of---set

It may not be that bad to try to follow the same syntax (modulo multiple value support), then users will be familiar with that. Open for ideas here.

[dmitryax]

looking at exact difference is not that bad "--set=" is replaced by "--config=yaml:", "." with "::", and "=" with ":", but let's assume it is that bad

Another inconvenience is that the argument value has to be always quoted because of the space in it --config="yaml:processors::batch::timeout: 2s", while --set doesn't need quotes most of the times --set =processors.batch.timeout=2s

[bogdandrutu]

@dmitryax is your proposal to do what helm does?

[codeboten]

I like the idea of following what --set does in helm and providing users with similar documentation to explain the limitations.

[dmitryax]

@dmitryax is your proposal to do what helm does?

I'm more inclined towards that, yes. I believe, if we decide to keep --set, we don't have to follow what we do in yaml, we can leave :: for yaml and use a bit different syntax for --set.

[bogdandrutu]

@codeboten @dmitryax awesome, #6295 moves us to that direction in a sense that we have the flag not forcing to respect the "properties" syntax as defined by java properties, and use our custom parser for that to be able to customize it.

[bogdandrutu]

Another inconvenience is that the argument value has to be always quoted because of the space in it --config="yaml:processors::batch::timeout: 2s", while --set doesn't need quotes most of the times --set =processors.batch.timeout=2s

@dmitryax that is also required for some "-set" values, see #6321 where you need quotes for complex values like array/maps.

[bogdandrutu]

@codeboten @dmitryax per the latest PR, current limitations are:

1. Does not support setting a key that contains a dot ..
2. Does not support setting a key that contains a equal sign =.
3. The configuration key separator inside the value part of the property is "::". For example --set "name={a::b: c}" is equivalent with --set name.a.b=c.

Bogdan's proposals:

For 1. I suggest to do the escaping similar with the helm story.

For 2. I suggest to do nothing since I believe that is not a problem.

For 3. I am not convinced, because it looks like inconsistency between key part separator and keys inside value separators, so I would tend to try to fix that, but I like that I don't have to escape "." in that part. Any idea?

[Aneurysm9]

For 2. I suggest to do nothing since I believe that is not a problem.

I assume we're not doing this anywhere today and would probably be wary of someone attempting to do so, but we should probably formalize it as a rule if we want to say it's not something that this implementation needs to be concerned with.

For 3. I am not convinced, because it looks like inconsistency between key part separator and keys inside value separators, so I would tend to try to fix that, but I like that I don't have to escape "." in that part. Any idea?

Can we define the value part of these pairs as "what would be valid for the yaml:// config provider"? It does feel a little weird, but I don't immediately hate it.

[jpkrohling]

I'm also happier with the Helm style. I'm having trouble understanding when the problems 1/2/3 would occur. Do you have concrete examples in mind? Here's what I could guess:

Problem 1: --set processors.attributes.headers.Header\.Dot=thevalue (meaning, Header.Dot is the name of the key on a hypothetical headers map of the attributes processor)
Problem 2: I can't think of a case where the = would be part of the key
Problem 3: I also can't think of such a case, but I'm sure you had something in mind for it

[bogdandrutu]

Problem 1: --set processors.attributes.headers.Header.Dot=thevalue (meaning, Header.Dot is the name of the key on a hypothetical headers map of the attributes processor)

See https://github.com/open-telemetry/opentelemetry-collector/blob/main/service/telemetry/config.go#L33. In the OTel resource we have lots of attributes with does.

Problem 2: I can't think of a case where the = would be part of the key

Me nighter, but it is a limitation, so that's why I think we just need to document it and live with it.

Problem 3: I also can't think of such a case, but I'm sure you had something in mind for it

The configuration key separator inside the value part of the property is "::". For example --set "name={a::b: c}" is equivalent with --set name.a.b=c.

This is a concrete limitation of our current implementation. So we may say the same as number 2, that we ignore this and live with it, but I documented them to make sure we understand the current implementation and that we will not be able to change this in the future.

[jpkrohling]

Thanks, it's clear now. I'm fine to live with those limitations.