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] Change config fields to use snake_case #32331

Merged
merged 4 commits into from
Apr 12, 2024

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Apr 11, 2024

Change AWS Cloud map resolver config fields from camelCase to snake_case.

The snake_case is required in OTel Collector config fields. It used to be enforced by tests in cmd/oteltestbedcol, but we had to disable them some time ago. Now, the tests are going to be enforced on every component independently. Hence, the camelCase config fields recently added with the new AWS Cloud Map resolver has to be fixed.

Copy link
Member

@songy23 songy23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrzej-stencel
Copy link
Member

I know it's a pain, but given that this component is in "Beta" stability, shouldn't we do the rename via a feature gate?

@dmitryax
Copy link
Member Author

The feature was introduced only in 0.97.0 #27588 so I expect it's not widely used yet.

And I want to unblock #32328 sooner to avoid any other issues like this merged in.

So I'd rather make it as a breaking change but happy to hear what others think cc @jpkrohling @open-telemetry/collector-contrib-approvers

@andrzej-stencel
Copy link
Member

The feature was introduced only in 0.97.0 #27588 so I expect it's not widely used yet.

Makes sense to me 🚀

Copy link
Member

@andrzej-stencel andrzej-stencel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README needs an update as well

@dmitryax dmitryax force-pushed the fix-load-balancer-exporter branch from a280f5c to f416b78 Compare April 11, 2024 21:21
@dmitryax
Copy link
Member Author

Updated README

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another reference that needs fixed (output of an error message):

errNoServiceName = errors.New("no Cloud Map serviceName specified to resolve the backends")

Some other references that should possibly be updated (log messages for config names):

r.logger.Info("AWS CloudMap resolver started",
zap.Stringp("serviceName", r.serviceName),
zap.Stringp("namespaceName", r.namespaceName),
zap.Uint16p("port", r.port),
zap.String("healthStatus", string(*r.healthStatus)),
zap.Duration("interval", r.resInterval), zap.Duration("timeout", r.resTimeout))

awsCloudMapLogger := params.Logger.With(zap.String("resolver", "awsCloudMap"))

exporter/loadbalancingexporter/README.md Outdated Show resolved Hide resolved
exporter/loadbalancingexporter/README.md Show resolved Hide resolved
exporter/loadbalancingexporter/README.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member Author

Thanks @crobert-1. Updated

namespace: aws-namespace
serviceName: aws-otel-col-service-name
service_name: aws-otel-col-service-name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
service_name: aws-otel-col-service-name
service_name: otel-col-service-name

Teeny tiny nit. Vendor agnostic right :)

Copy link
Member Author

@dmitryax dmitryax Apr 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I change aws-namespace as well? It's pretty hard to make AWS Cloud Map resolver vendor agnostic :)

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is the last reference, but not necessarily essential:

awsCloudMapLogger := params.Logger.With(zap.String("resolver", "awsCloudMap"))

dmitryax and others added 4 commits April 11, 2024 19:43
Change AWS Cloud map resolver config fields from camelCase to snake_case.

The snake_case is required in OTel Collector config fields. It used to be enforced by tests in cmd/oteltestbedcol, but we had to disable them some time ago. Now, the tests are going to be enforced on every component independently. Hence, the camelCase config fields recently added with the bew AWS Cloud Map resolver has to be fixed.
@dmitryax dmitryax force-pushed the fix-load-balancer-exporter branch from 75e869c to 105494b Compare April 12, 2024 02:43
@dmitryax dmitryax merged commit 960844a into open-telemetry:main Apr 12, 2024
170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 12, 2024
@dmitryax dmitryax deleted the fix-load-balancer-exporter branch April 12, 2024 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants