Update the builder to use a consistent binary name. #5651

jpeach · 2022-07-08T02:13:47Z

Description:

Update the builder to consistently use "ocb" in the name of the command,
version and configuration file.

Link to tracking Issue:

This fixes #5581.

Testing:

Manually looked at version output.

Documentation:

None

Makefile

cmd/builder/internal/command.go

TylerHelmuth · 2022-07-08T15:14:09Z

cmd/builder/internal/command.go

@@ -59,7 +59,7 @@ func Command() (*cobra.Command, error) {
 	}

 	// the external config file
-	cmd.Flags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.otelcol-builder.yaml)")
+	cmd.Flags().StringVar(&cfgFile, "config", "$HOME/.ocb.yaml", "config file (default is $HOME/.ocb.yaml)")


This is a breaking change right? Is it ok to break this in 1 release or do we need to take a deprecated approach? I'm thinking of situations where users of the ocb are already taking advantage of the default .otelcol-builder.yaml file and that would stop working when this is released. Could break CICD pipelines.

Yeh, it's a breaking change. We could revert (at the cost of some inconsistency), or we could fall back to .otelcol-builder.yaml if .ocb.yaml isn't present.

Medium term, I'm wondering whether it makes sense to take the config file only as an argument, not a flag, but I need to do some more research and learning beforehand so that I can make a good proposal :)

Falling back to .otelcol-builder.yaml and adding a warning about it being deprecated in the future seems like a safe way to go.

Since we need to add a fallback, what I propose is

deprecate the required--config flag

retain the current --config flag implementation for backward compatibility

default to requiring a single argument that is the path to the config file to build

From my perspective, the default $HOME location for config isn't that useful, since the config ought to be under source control. Compulsory options are better expressed as arguments than flags.

I'll push an update later today.

I am good with this approach, but it does deviate from the collector's ergonomics of passing in configuration via a --config flag.

does deviate from the collector's ergonomics of passing in configuration via a --config flag

Yep, IMHO distinction is that ocb is a compiler that takes a config input and generates a corresponding output, whereas the collector is a long-running service, where it's config is part of the system.

cmd/builder/internal/command.go

codeboten

Thanks for fixing the inconsistencies! Just a few suggestions, please add an entry to the changelog as well

codeboten · 2022-07-15T14:30:17Z

cmd/builder/internal/command.go

@@ -59,7 +59,7 @@ func Command() (*cobra.Command, error) {
 	}

 	// the external config file
-	cmd.Flags().StringVar(&cfgFile, "config", "", "config file (default is $HOME/.otelcol-builder.yaml)")
+	cmd.Flags().StringVar(&cfgFile, "config", "$HOME/.ocb.yaml", "config file (default is $HOME/.ocb.yaml)")


Falling back to .otelcol-builder.yaml and adding a warning about it being deprecated in the future seems like a safe way to go.

cmd/builder/internal/command.go

jpeach · 2022-07-19T05:14:14Z

Using the flag:

$ ./builder --config=./b.conf
Flag --config has been deprecated, pass the build configuration as the first argument instead
2022-07-19T15:11:53.755+1000	INFO	internal/command.go:119	OpenTelemetry Collector Builder	{"version": "dev", "date": "unknown"}
2022-07-19T15:11:53.756+1000	INFO	internal/command.go:138	Using config file	{"path": "./b.conf"}
2022-07-19T15:11:53.756+1000	INFO	builder/config.go:99	Using go	{"go-executable": "/opt/homebrew/bin/go"}
2022-07-19T15:11:53.756+1000	INFO	builder/main.go:76	Sources created	{"path": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1100039365"}
2022-07-19T15:11:53.984+1000	INFO	builder/main.go:108	Getting go modules
2022-07-19T15:11:54.017+1000	INFO	builder/main.go:87	Compiling
2022-07-19T15:11:55.675+1000	INFO	builder/main.go:94	Compiled	{"binary": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1100039365/otelcol-custom"}

Using the arg:

$ ./builder b.conf
2022-07-19T15:12:24.721+1000	INFO	internal/command.go:119	OpenTelemetry Collector Builder	{"version": "dev", "date": "unknown"}
2022-07-19T15:12:24.721+1000	INFO	internal/command.go:138	Using config file	{"path": "b.conf"}
2022-07-19T15:12:24.721+1000	INFO	builder/config.go:99	Using go	{"go-executable": "/opt/homebrew/bin/go"}
2022-07-19T15:12:24.722+1000	INFO	builder/main.go:76	Sources created	{"path": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1049441039"}
2022-07-19T15:12:24.918+1000	INFO	builder/main.go:108	Getting go modules
2022-07-19T15:12:24.945+1000	INFO	builder/main.go:87	Compiling
2022-07-19T15:12:25.740+1000	INFO	builder/main.go:94	Compiled	{"binary": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1049441039/otelcol-custom"}

Using both:

$ ./builder --config=b.conf b.conf
Flag --config has been deprecated, pass the build configuration as the first argument instead
Error: configuration flag and argument must not both be provided
Usage:
  ocb [flags] FILE
  ocb [command]

Available Commands:
  completion  Generate the autocompletion script for the specified shell
  help        Help about any command
  version     Version of ocb

Flags:
      --description string       A descriptive name for the OpenTelemetry Collector distribution (default "Custom OpenTelemetry Collector distribution")
      --go string                The Go binary to use during the compilation phase. Default: go from the PATH
  -h, --help                     help for ocb
      --module string            The Go module for the new distribution (default "go.opentelemetry.io/collector/cmd/builder")
      --name string              The executable name for the OpenTelemetry Collector distribution (default "otelcol-custom")
      --otelcol-version string   Which version of OpenTelemetry Collector to use as base (default "0.55.0")
      --output-path string       Where to write the resulting files (default "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1945253626")
      --skip-compilation         Whether builder should only generate go code with no compile of the collector (default false)
      --version string           The version for the OpenTelemetry Collector distribution (default "1.0.0")

Use "ocb [command] --help" for more information about a command.

Error: configuration flag and argument must not both be provided

Using none:

$ ./builder
2022-07-19T15:13:17.637+1000	INFO	internal/command.go:119	OpenTelemetry Collector Builder	{"version": "dev", "date": "unknown"}
2022-07-19T15:13:17.637+1000	ERROR	internal/command.go:123	failed to load config file	{"config-file": "$HOME/.otelcol-builder.yaml", "error": "open $HOME/.otelcol-builder.yaml: no such file or directory"}
go.opentelemetry.io/collector/cmd/builder/internal.initConfig
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/internal/command.go:123
go.opentelemetry.io/collector/cmd/builder/internal.Command.func1
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/internal/command.go:72
github.com/spf13/cobra.(*Command).execute
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:872
github.com/spf13/cobra.(*Command).ExecuteC
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:990
github.com/spf13/cobra.(*Command).Execute
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:918
main.main
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/main.go:26
runtime.main
	/opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/proc.go:250
2022-07-19T15:13:17.637+1000	INFO	internal/command.go:138	Using config file	{"path": "$HOME/.otelcol-builder.yaml"}
2022-07-19T15:13:17.637+1000	INFO	builder/config.go:99	Using go	{"go-executable": "/opt/homebrew/bin/go"}
2022-07-19T15:13:17.638+1000	INFO	builder/main.go:76	Sources created	{"path": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1733631529"}
2022-07-19T15:13:17.807+1000	INFO	builder/main.go:108	Getting go modules
2022-07-19T15:13:17.835+1000	INFO	builder/main.go:87	Compiling
2022-07-19T15:13:18.649+1000	INFO	builder/main.go:94	Compiled	{"binary": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1733631529/otelcol-custom"}

If this everyone agrees and this is merged, I'll also update the main website docs on the next release.

jpkrohling · 2022-07-19T13:46:33Z

2022-07-19T15:13:17.637+1000	ERROR	internal/command.go:123	failed to load config file	{"config-file": "$HOME/.otelcol-builder.yaml", "error": "open $HOME/.otelcol-builder.yaml: no such file or directory"}
go.opentelemetry.io/collector/cmd/builder/internal.initConfig
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/internal/command.go:123
go.opentelemetry.io/collector/cmd/builder/internal.Command.func1
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/internal/command.go:72
github.com/spf13/cobra.(*Command).execute
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:872
github.com/spf13/cobra.(*Command).ExecuteC
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:990
github.com/spf13/cobra.(*Command).Execute
	/Users/jpeach/go/pkg/mod/github.com/spf13/[email protected]/command.go:918
main.main
	/Users/jpeach/upstream/opentelemetry/opentelemetry-collector/cmd/builder/main.go:26
runtime.main
	/opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/proc.go:250

This should not happen. An info or warn-level message should be logged that the default config is being used, not an error with a stack trace.

jpeach · 2022-07-19T23:12:42Z

This should not happen. An info or warn-level message should be logged that the default config is being used, not an error with a stack trace.

This is the standard ocb behavior when the config file isn't found. There's a couple of things I can do here:

log deprecation warning when we do the fallback (I didn't do that because the logging hasn't started at the point where we fall back, but can add it if we want)
only fall back if the file exists
log the error, but no stack

jpeach · 2022-07-20T04:38:31Z

This should not happen. An info or warn-level message should be logged that the default config is being used, not an error with a stack trace.

@codeboten @jpkrohling

I verified with release build from github, and there is no code to expand the default $HOME/.otelcol-builder.yaml, so the default has never worked AFAIK.

So I can revert all the args changes and just make the --config flag required. Alternatively, I can press on and remove the default since there’s no backwards compatibility issue AFAICT. LMK what approach you prefer.

jpkrohling · 2022-07-20T12:04:28Z

I verified with release build from github, and there is no code to expand the default $HOME/.otelcol-builder.yaml, so the default has never worked AFAIK.

I was sure it used to work, so I picked an old version (0.45.0) and ran git bisect, which pointed me to the culprit:

78e9640760fc6b0d04cb454b6f94fcb4fc22f647 is the first bad commit
commit 78e9640760fc6b0d04cb454b6f94fcb4fc22f647
Author: Ryan Leung <[email protected]>
Date:   Thu Feb 24 03:26:15 2022 +0800

    Replace viper with koanf in builder (#4757)
    
    * replace viper with koanf in builder
    
    Signed-off-by: Ryan Leung <[email protected]>
    
    * resolve the conflicts
    
    Signed-off-by: Ryan Leung <[email protected]>
    
    * Update go.sum
    
    Co-authored-by: Bogdan Drutu <[email protected]>
    Co-authored-by: Bogdan Drutu <[email protected]>

 cmd/builder/go.mod              | 13 ++------
 cmd/builder/go.sum              | 67 +++++++++++++++++++++++++++++++++++------
 cmd/builder/internal/command.go | 48 ++++++++++++++---------------
 3 files changed, 85 insertions(+), 43 deletions(-)

This means this has not been working since February and we just found out about it. I agree there's no backwards compatibility problem here then. I would be OK with not having a default location for the file, but we should still have a reasonable default configuration, so that the naked command still produces a reasonable binary (like, one with OTLP components).

bogdandrutu · 2022-07-20T22:21:25Z

cmd/builder/internal/command.go

+	// Since ocb is a compiler, the input file (if present) should
+	// be the first (and only) command argument. This flag is preserved
+	// for backwards compatibility with pre-1.0 releases.
+	if err := cmd.Flags().MarkDeprecated("config", "pass the build configuration as the first argument instead"); err != nil {
+		panic(err) // Only fails if the usage message is empty, which is a programmer error.
+	}


Why this change? Unrelated with PR description and issue. Please fix the PR to only do one thing.

jpeach · 2022-07-20T23:26:26Z

This means this has not been working since February and we just found out about it. I agree there's no backwards compatibility problem here then. I would be OK with not having a default location for the file

@jpkrohling OK, so I propose to revert all the args changes, and just update the --config flag to be required and have no default. Given the history here, that is effectively backwards compatible.

but we should still have a reasonable default configuration, so that the naked command still produces a reasonable binary

Since the regression you found, anyone depending on the default would get silently incorrect results. Because the binary that is produced without any configuration doesn't have any components, I think it's better to always require the --config flag for now.

I can post a follow-up PR that relaxes that and uses the cmd/otelcorecol/builder-config.yaml as the default if no config is specified. WDYT?

jpkrohling · 2022-07-21T12:34:46Z

I can post a follow-up PR that relaxes that and uses the cmd/otelcorecol/builder-config.yaml as the default if no config is specified. WDYT?

Sounds good.

Update the builder to consistently use "ocb" in the name of the command, version. Since the builder does not expand environment variables in the config file pathname, the default value for `--config` does not work, so we can make this flag required so that users don't silently build an inoperative collector. This fixes open-telemetry#5581. Signed-off-by: James Peach <[email protected]>

jpeach · 2022-07-22T01:16:20Z

I can post a follow-up PR that relaxes that and uses the cmd/otelcorecol/builder-config.yaml as the default if no config is specified. WDYT?

Sounds good.

Done, and pushed the change.

codecov · 2022-07-22T12:40:00Z

Codecov Report

Merging #5651 (b28413c) into main (c3a3160) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #5651   +/-   ##
=======================================
  Coverage   91.53%   91.53%           
=======================================
  Files         192      192           
  Lines       11398    11411   +13     
=======================================
+ Hits        10433    10445   +12     
- Misses        770      771    +1     
  Partials      195      195

Impacted Files	Coverage Δ
exporter/exporterhelper/queued_retry_inmemory.go	`91.89% <0.00%> (-3.70%)`	⬇️
pdata/internal/common.go	`91.85% <0.00%> (-0.75%)`	⬇️
exporter/exporterhelper/obsreport.go	`100.00% <0.00%> (ø)`
config/confighttp/compression.go	`84.11% <0.00%> (+5.81%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3a3160...b28413c. Read the comment docs.

jpeach requested review from a team and dmitryax July 8, 2022 02:13

jpkrohling reviewed Jul 8, 2022

View reviewed changes

Makefile Show resolved Hide resolved

cmd/builder/internal/command.go Show resolved Hide resolved

TylerHelmuth reviewed Jul 8, 2022

View reviewed changes

jpeach force-pushed the consistent-builder-name branch from aa1fb56 to 6bd2d58 Compare July 9, 2022 02:40

codeboten reviewed Jul 15, 2022

View reviewed changes

jpeach force-pushed the consistent-builder-name branch from 6bd2d58 to 563d003 Compare July 19, 2022 05:11

jpeach force-pushed the consistent-builder-name branch from 563d003 to 3bcf6e4 Compare July 20, 2022 05:23

bogdandrutu reviewed Jul 20, 2022

View reviewed changes

jpeach force-pushed the consistent-builder-name branch from 3bcf6e4 to b28413c Compare July 22, 2022 01:13

jpkrohling approved these changes Jul 22, 2022

View reviewed changes

bogdandrutu added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 22, 2022

bogdandrutu merged commit 3e14ee9 into open-telemetry:main Jul 22, 2022

mx-psi mentioned this pull request Sep 14, 2022

[docs] Update release instructions #6068

Merged

This was referenced Sep 28, 2022

[receiver/otlp] Rewrite logs related to server startup #6174

Merged

[pdata] Update deprecation comments #6206

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the builder to use a consistent binary name. #5651

Update the builder to use a consistent binary name. #5651

jpeach commented Jul 8, 2022

TylerHelmuth Jul 8, 2022

jpeach Jul 9, 2022

codeboten Jul 15, 2022

jpeach Jul 17, 2022

TylerHelmuth Jul 18, 2022

jpeach Jul 19, 2022

codeboten left a comment

codeboten Jul 15, 2022

jpeach commented Jul 19, 2022 •

edited

Loading

jpkrohling commented Jul 19, 2022

jpeach commented Jul 19, 2022

jpeach commented Jul 20, 2022 •

edited

Loading

jpkrohling commented Jul 20, 2022 •

edited

Loading

bogdandrutu Jul 20, 2022

jpeach commented Jul 20, 2022

jpkrohling commented Jul 21, 2022

jpeach commented Jul 22, 2022

codecov bot commented Jul 22, 2022

Update the builder to use a consistent binary name. #5651

Update the builder to use a consistent binary name. #5651

Conversation

jpeach commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeboten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Jul 19, 2022 • edited Loading

jpkrohling commented Jul 19, 2022

jpeach commented Jul 19, 2022

jpeach commented Jul 20, 2022 • edited Loading

jpkrohling commented Jul 20, 2022 • edited Loading

Choose a reason for hiding this comment

jpeach commented Jul 20, 2022

jpkrohling commented Jul 21, 2022

jpeach commented Jul 22, 2022

codecov bot commented Jul 22, 2022

Codecov Report

jpeach commented Jul 19, 2022 •

edited

Loading

jpeach commented Jul 20, 2022 •

edited

Loading

jpkrohling commented Jul 20, 2022 •

edited

Loading