Skip to content

Any uppercase character in config file is forced to be lowercase. #2594

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

Closed
gillg opened this issue Mar 3, 2021 · 11 comments · Fixed by #3337
Closed

Any uppercase character in config file is forced to be lowercase. #2594

gillg opened this issue Mar 3, 2021 · 11 comments · Fixed by #3337
Assignees
Labels
area:config bug Something isn't working priority:p1 High release:required-for-ga Must be resolved before GA release

Comments

@gillg
Copy link
Contributor

gillg commented Mar 3, 2021

Describe the bug

For an exporter (Loki), but this issue is more global, I would target an OTEL attribute named "EventType" to forward it as loki label.
To do that, I have to create a section in OTEL collector config file like:

exporters:
  loki:
    attributes:
      labels:
        EventType: ""

The name "EventType" is validated by loki exporter regarding prometheus LabelName regex, everything is fine in theory.
But in fact it doesn't work, without any error, because the main "Config" class on OTEL collector unmarshal all the yaml file as lowercase.

It seems an old bug referenced here spf13/viper#373 between the Viper lib and mapstructure.
I'm definitely not confortable with Go but as I understand, starting to this point, https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/config.go#L138 all your config file is altered and normalized in lowercase even if it's not a YAML limitation or an otel collector needs.

I think this can produce unexpected silent issues with processor if you try to enrich some attributes not fully in lowercase...

There is workarounds like copy needed attributes to a lowercase version of them, or force lowercase comparaison here https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/fa600292107a7b9f32ae5bf9f76d5c663e22ba83/exporter/lokiexporter/exporter.go#L171 for example.

But in every cases it's not the normal use case.
I don't see lot of plans except maybe replace the viper lib or contribute to fix it.

Steps to reproduce
Add any uppuer char in processors / exporters sections

What did you expect to see?
The case should be preserved at this point https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/config.go#L138 in rawCfg var

What did you see instead?
My YAML map keys are forced to lowercase

What version did you use?
compilation of https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/fa600292107a7b9f32ae5bf9f76d5c663e22ba83

What config did you use?

receivers:
  fluentforward:
    endpoint: 0.0.0.0:8006

exporters:
  loki:
    endpoint: http://loki:3100/loki/api/v1/push
    # Otel Attributes keys to send as "label" on loki
    labels:
      attributes:
        severity: ""
        # EventType attribute is renamed in lower case automaticaly...
        # No way to trick because the OTEL attribute is really EventType so it doesn't match
        EventType: ""
        # Allow "hostname" and rename it to severity
        hostname: "instance"
        instance: ""
        job: ""

processors:
  batch:

service:
  pipelines:
    logs:
      receivers: [fluentforward]
      processors: [batch]
      exporters: [loki]
@gillg gillg added the bug Something isn't working label Mar 3, 2021
@bogdandrutu
Copy link
Member

@gillg does using "EventType" in the config work for you? I think that is not a key from config perspective is a key in a map[string]Value

@gillg
Copy link
Contributor Author

gillg commented Mar 3, 2021

@bogdandrutu I'm not sure to understand your question ?
This issue is because EventType doesn't work as expected. I tried with double quotes arround (if it was your question ?) and it's the same result.

I have an OTEL Attribute with the exact key EventType, but I can't write config with YAML keys equals to it.
In fact the config is valid but forced to lower case when OTEL collector loads it. So my rule never match with my attribute.

Reading some docs, this issues was hidden from now, because most of receivers / processors / exporters uses "static" (in lowercase) YAML keys. For example some words like "match", "endpoint", "replace" and their values can be in any case.

But I reproduce it really simply with:

receivers:
  fluentforward:
    endpoint: 0.0.0.0:8006

processors:
  batch/lower:
  batch/Upper:

service:
  pipelines:
    logs:
      receivers: [fluentforward]
      processors: [batch/lower, batch/Upper]

Result : Error: cannot load configuration: pipeline "logs" references processor batch/Upper which does not exist

Because the YAML key batch/Upper was replaced by batch/upper in the config object in memory.

@bogdandrutu
Copy link
Member

@tigrannajaryan this probably needs to be consider as part of our phase1, to ensure that if we cannot use Viper because of these problems we do not expose it in our public APIs.

@tigrannajaryan
Copy link
Member

Yeah, this is pretty bad and has been a problem with Viper for a very long time now. I agree @bogdandrutu we probably need to hide it as an implementation detail so that we can fix it ourselves without breaking components.

@punya
Copy link
Member

punya commented Mar 26, 2021

Can we work around this type of problem by agreeing to use a list of {key, value} items for cases where we need key case preserved rather than normalized according to Viper's rules. i.e.,

exporters:
  loki:
    endpoint: http://loki:3100/loki/api/v1/push
    # Otel Attributes keys to send as "label" on loki
    labels:
      attributes:
        - key: severity
          value: ""
        - key: EventType
          value: ""
        - key: hostname
          value: instance
        - key: instance
          value: 
        - key: job
          value: ""

As a variation on this idea, we could use a config format that is more explicit about the mapping operation

exporters:
  loki:
    endpoint: http://loki:3100/loki/api/v1/push
    # Otel Attributes keys to send as "label" on loki
    labels:
      attributeMapping:
        drop:
        - severity
        - EventType
        - instance
        - job
        rename:
        - from: hostname
          to: instance

@punya
Copy link
Member

punya commented Mar 26, 2021

If we find a way to stop Viper from normalizing case, we should be cautious about how we roll it out, because people may already by relying on the existing behavior (knowingly or unknowingly).

@mx-psi
Copy link
Member

mx-psi commented Apr 14, 2021

We can add an option when creating the new config.Parser struct to specify if one wants to load the configuration in a case-sensitive or case-insensitive way (keeping it case-insensitive as a default). I think that would work nicely.

We need however to settle on #2651 first and, if moving away from Viper, we may want to remove the custom unmarshaling fallback here first so that we can remove this method

// Viper returns the viper.Viper implementation of this Parser.
func (l *Parser) Viper() *viper.Viper {
return l.v
}

@alolita alolita added area:config priority:p1 High release:required-for-ga Must be resolved before GA release labels May 12, 2021
@alolita
Copy link
Member

alolita commented May 18, 2021

@tigrannajaryan @bogdandrutu are you working on this issue. Please advise.

@tigrannajaryan
Copy link
Member

Some work was done to decouple the implementation from Viper. We still need to replace it by something else that is case-sensitive. I am not working on this and AFAIK Bogdan is not either at the moment.

@alolita
Copy link
Member

alolita commented May 20, 2021

Thanks for the update @tigrannajaryan! I will assign this issue to both of you for the time being given this is work in progress. Let me know if anything changes on your end.

@tigrannajaryan
Copy link
Member

I suggest to push this to Phase 2 or even post GA. The fix should not be a breaking change. @bogdandrutu thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:config bug Something isn't working priority:p1 High release:required-for-ga Must be resolved before GA release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants