Skip to content

fix: always re-add config values provided with --config after internal config values #164

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

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

obs-gh-mattcotter
Copy link
Collaborator

Description

After switching to the otelcol's internal --config flag, the bundled config files provided by the observe-agent now are processed after any configuration provided with --config. This makes it so the bundled config values cannot be overwritten via the --config flag (only via the otel_config_overrides option). This fix makes the flag take precedence again.

Ex:

$ cat ./hc_test.yaml 
extensions:
  health_check:
    endpoint: 0.0.0.0:13137

Current 2.0.0:

$ observe-agent start --config ./hc_test.yaml 
# ...
2025-02-10T11:10:54.862-0600	info	healthcheckextension/healthcheckextension.go:32	Starting health_check extension	{"kind": "extension", "name": "health_check", "config": {"Endpoint":"localhost:13133","TLSSetting":null,"CORS":null,"Auth":null,"MaxRequestBodySize":0,"IncludeMetadata":false,"ResponseHeaders":null,"CompressionAlgorithms":null,"ReadTimeout":0,"ReadHeaderTimeout":0,"WriteTimeout":0,"IdleTimeout":0,"Path":"/status","ResponseBody":null,"CheckCollectorPipeline":{"Enabled":false,"Interval":"5m","ExporterFailureThreshold":5}}}

^ note the "Endpoint":"localhost:13133"

After this fix:

$ ./observe-agent start --config ./hc_test.yaml 
# ...
2025-02-10T11:11:41.227-0600	info	healthcheckextension/healthcheckextension.go:32	Starting health_check extension	{"kind": "extension", "name": "health_check", "config": {"Endpoint":"0.0.0.0:13137","TLSSetting":null,"CORS":null,"Auth":null,"MaxRequestBodySize":0,"IncludeMetadata":false,"ResponseHeaders":null,"CompressionAlgorithms":null,"ReadTimeout":0,"ReadHeaderTimeout":0,"WriteTimeout":0,"IdleTimeout":0,"Path":"/status","ResponseBody":null,"CheckCollectorPipeline":{"Enabled":false,"Interval":"5m","ExporterFailureThreshold":5}}}

^ note the "Endpoint":"0.0.0.0:13137"

Checklist

  • Created tests which fail without the change (if possible)
  • Extended the README / documentation, if necessary

@@ -115,7 +115,7 @@ locals {

WINDOWS_SERVER_2022_BASE = {
ami_instance_type = "t3.small"
ami_id = "ami-00bedb8509ebcc120"
Copy link
Collaborator

Choose a reason for hiding this comment

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

why are these guys changing?

Copy link
Collaborator Author

@obs-gh-mattcotter obs-gh-mattcotter Feb 10, 2025

Choose a reason for hiding this comment

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

Terraform couldn't find the old AMIs. The published images change from time to time, but I don't know why AWS does that.

// see: https://github.com/open-telemetry/opentelemetry-collector/blob/v0.118.0/otelcol/flags.go#L28-L30
// Trim the surrounding brackets, then split on ", "
var originalConfigs []string
if len(originalConfigStr) > 2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this part is sort of confusing/seems unfortunate to me. are the first 2 arguments special in some way?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is referring to the string; two characters only means [] which means no args. This skips the string split when we don't need to do it basically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh and one more weird thing: calling Split on empty string does not return empty array but instead returns array of empty string.
https://go.dev/play/p/wZ3He29DP37

So this saves from filtering out empty string when looping over the configs below.

@obs-gh-mattcotter obs-gh-mattcotter merged commit d37d19e into main Feb 11, 2025
8 checks passed
@obs-gh-mattcotter obs-gh-mattcotter deleted the mc/config-order branch February 11, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants