fix: changes default configs to work with docker-compose setup

[JBAhire]

Description

• collector image build with current configs doesn't work with docker-compose setup out of the box and needs manual overriding configs like this: https://github.com/hypertrace/hypertrace/blob/main/docker/configs/hypertrace-collector/config.yml
• As we use docker based e2e test in multiple repositories maintaining these config files at every place is hard.
• That's why changing the default configs in a way it will work with docker-compose setup without any overrides.

Testing

Tested with docker-compose setup locally.

Checklist:

• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• Any dependent changes have been merged and published in downstream modules

[kotharironak]

Should be good to have bootstrap:9092 and adding bootstrap as another alias in docker-compose?

[kotharironak]

Looks very specific config as default, and if someone wants to use our collector out-of-box, would make it difficult?

[kotharironak]

Can config folders have more than one config file as part of an image? And docker-compose setup can use the one which requires for HT. So, that way, it can work in default as well as along with HT bypassing config argument.

So, I would suggest, let's have one more config file in repo along with default-config.yml, say default-ht-config.yml. And copy this config file as in docker image after line (https://github.com/hypertrace/hypertrace-collector/blob/main/Dockerfile#L18)

COPY default-ht-config.yml /etc/opt/hypertrace/default-ht-config.yml

And in docker-compose setup lets override only config argument as

CMD ["--config", "/etc/opt/hypertrace/default-ht-config.yml"]

[JBAhire]

Done

[jcchavezs]

I am not sure about the point you make @kotharironak. I feel like in most of the cases people will use the collector with hypertrace platform and in some cases people might try this collector because of the PII filter we are adding and in that case they need to touch the config. So I'd provide the default HT config and then let people pass other configs.

[kotharironak]

If, that is the case, should be fine. But, the default config will still be very specific to docker-compose setup

• kafka-zookeeper (we still have to override with helm deployment). So, we can change it to bootstrap, and add one more alias in docker-compose.
• jaeger-spans (this should be fine, and can be considered as our default entry point)

[JBAhire]

@kotharironak , this configs are not gonna affect helm in any way. there's separate configmap for helm.

[jcchavezs]

kafka-zookeeper (we still have to override with helm deployment). So, we can change it to bootstrap, and add one more alias in docker-compose.

As per open-telemetry/opentelemetry-collector@047b0f3 we can use env var for that I think.

[JBAhire]

@jcchavezs , I made ht-config as default while passing in command.

[jcchavezs]

I am sorry I did not explain myself correctly. What I meant was that we could set the Kafka host over an env car instead of sticking to a particular name as this is very specific to docker compose setup.

…

On Thu, 4 Mar 2021, 09:54 Jayesh Bapu Ahire, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In default-config.yml <#45 (comment)> : > zipkin: processors: batch: exporters: - logging: - logLevel: debug + kafka: + protocol_version: 2.0.0 + brokers: + - kafka-zookeeper:9092 + topic: jaeger-spans @jcchavezs <https://github.com/jcchavezs> , I made ht-config as default while passing in command. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#45 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAXOYARCIGFQT5WRH2ASUPLTB5DEHANCNFSM4YSS53JA> .

[JBAhire]

@jcchavezs , @kotharironak, I added broker name as environment variable in ht-config file.

[kotharironak]

I think you want to continue with,

CMD ["--config", "/etc/opt/hypertrace/config.yml"]

[JBAhire]

#45 (comment) I was going with @jcchavezs 's comment here.

[kotharironak]

Then, do we need one more config default-config.yml file? As we are going with ENV, let's just have one file then.

[JBAhire]

default-config.yml has prometheus and other things which we don't use in docker-compose so it basically exits at that point.

I will keep them separate and let people use default-configs and tweak them according to need

[jcchavezs]

I agree if we use env vars we don't need to have two separated files for default config.

[kotharironak]

Suggested change

	CMD ["--config", "/etc/opt/hypertrace/ht-config.yml"]
	CMD ["--config", "/etc/opt/hypertrace/config.yml"]

[kotharironak]

Is there a way to keep the default value?

${KAFKA_BROKERS:-kafka-zookeeper}

If, so, add even for topic as well
topic: ${KAFKA_TOPIC:-jaeger-spans}

And also take the entire string - ${BROKER:-bootstrap:9092}

[JBAhire]

It doesn't work with assigning default values. sample config here suggests the same.

So, we need to stick to using static environment variables, I am guessing.

hypertrace-collector    | 2021/03/04 13:05:21 application run finished with error: cannot setup pipelines: cannot build builtExporters: error creating kafka exporter: kafka: client has run out of available brokers to talk to (Is your cluster reachable?)

[jcchavezs]

Yeah I was thinking the same. I will work this out open-telemetry/opentelemetry-collector#2534.

[jcchavezs]

Thanks @JBAhire !