Skip to content
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

[exporter/loadbalancing] The awsCloudMap tag doesn't satisfy the config tag regular expression #32296

Closed
elikatsis opened this issue Apr 10, 2024 · 3 comments
Labels
bug Something isn't working exporter/loadbalancing needs triage New item requiring triage

Comments

@elikatsis
Copy link
Contributor

elikatsis commented Apr 10, 2024

Component(s)

exporter/loadbalancing

What happened?

Description

The loadbalancing exporter feature to enable using the AWS service discovery system described in #27241 and implemented by #27588 introduced a config field with tag awsCloudMap which is using camelCase as well as two more sub-fields in camelCase: serviceName and healthStatus.

However, config tags need to satisfy the following regex: ^[a-z0-9][a-z0-9_]*$
This is declared in https://github.com/open-telemetry/opentelemetry-collector/blob/de3ef01bff4f1e4db0eee94abce862c66582f208/component/componenttest/configtest.go#L15-L16.

The regular expression does not allow caps, so the tag should be aws_cloud_map instead.

In my team, we build our custom OTEL collector, heavily based on the otelcol-contrib one, and as part of our CI we run the tests for all Go modules including the OTEL collector (auto-generated module with ocb).
This bug breaks components_test.go and it breaks our CI.
We temporarily dropped the loadbalancing exporter from our OTEL collector, but it would be great if we included it back soon.

Steps to Reproduce

  1. Clone the https://github.com/open-telemetry/opentelemetry-collector-releases repository
    ~$ git clone https://github.com/open-telemetry/opentelemetry-collector-releases
    ~$ cd opentelemetry-collector-releases
    
  2. (Optionally) Check out to the latest release v0.97.0
    ~/opentelemetry-collector-releases$ git checkout v0.97.0
    
  3. Ensure you have the latest ocb
    ~/opentelemetry-collector-releases$ ocb version
    ocb version 0.97.0
    
    If it is not the latest (v0.97.0), create a backup of your existing ocb binary and install the correct one
    ~/opentelemetry-collector-releases$ mv $(which ocb){,.bak}
    ~/opentelemetry-collector-releases$ make ocb
    
  4. Generate the sources for otelcol-contrib
    ~/opentelemetry-collector-releases$ DISTRIBUTIONS=otelcol-contrib make generate-sources
    
  5. Run the tests of the generated sources
    ~/opentelemetry-collector-releases$ cd distributions/otelcol-contrib/_build/
    ~/opentelemetry-collector-releases/distributions/otelcol-contrib/_build/$ go test -v ./...
    ... FAIL ... (see below)
    

Expected Result

Successfull tests.

Actual Result

~/git/opentelemetry-collector-releases/distributions/otelcol-contrib/_build$ go test -v ./...
=== RUN   TestValidateConfigs
    components_test.go:27: 
                Error Trace:    /home/elikatsis/git/opentelemetry-collector-releases/distributions/otelcol-contrib/_build/components_test.go:27
                Error:          Received unexpected error:
                                type "Config" from package "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter" has invalid config settings: type "ResolverSettings" from package "github.com/open-telemetry/opentelemetry-collector-contrib/exporter/loadbalancingexporter" has invalid config settings: field "AWSCloudMap" has config tag "awsCloudMap" which doesn't satisfy "^[a-z0-9][a-z0-9_]*$"
                Test:           TestValidateConfigs
--- FAIL: TestValidateConfigs (0.00s)
FAIL
FAIL    github.com/open-telemetry/opentelemetry-collector-releases/contrib      0.072s
FAIL

The awsCloudMap config tag doesn't satisfy the regular expression. Changing it to aws_cloud_map fixes the issue.
The serviceName and healthStatus fields don't need to change, the validation logic doesn't check the sub-structs and I guess it's due to the fact that AWSCloudMap is a pointer to AWSCloudMapResolver (and the validation logic doesn't expand pointers)

Collector version

v0.97.0

Environment information

Environment

OS: Ubuntu 22.04
Compiler: go 1.21.0

OpenTelemetry Collector configuration

No response

Log output

No response

Additional context

I tried changing the config tag regular expression to allow capitalized letters and it fixes the tests. However, I think that it's more intrusive to change the regular expression in the core repository than to fix the config tag in this exporter.

The regular expression explicitly allows only lowercase snake_case config tags. Changing it to also allow capitalized letters will break uniformity.
At the same time, I understand that changing the awsCloudMap tag to aws_cloud_map is an API change which needs extra care.

I started the issue to gather opinions on how you people prefer to proceed with this:

  1. Patch https://github.com/open-telemetry/opentelemetry-collector or
  2. Change the exporter's API to aws_cloud_map
    • and optionally change the sub-fields to service_name and health_status for uniformity

Mentioning @andretong who introduced (and probably uses) the feature.
Also mentioning @jpkrohling who is the component's owner.

@elikatsis elikatsis added bug Something isn't working needs triage New item requiring triage labels Apr 10, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@elikatsis
Copy link
Contributor Author

/label discussion-needed

@crobert-1
Copy link
Member

This should be resolved by #32331 in the next collector release, v0.99.0.

Thanks for filing and bringing this to our attention @elikatsis! Appreciate your thoroughness here 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working exporter/loadbalancing needs triage New item requiring triage
Projects
None yet
Development

No branches or pull requests

2 participants