Add a default build configuration to ocb. #5752

jpeach · 2022-07-26T06:05:50Z

Description:

Embed the build configuration that is used to build otelcorecol into ocb
so that end users can easily generate a useful collector. This makes the
--config flag optional again.

Link to tracking Issue:

None.

Testing:

$ make ocb
/Library/Developer/CommandLineTools/usr/bin/make -C cmd/builder config
sed '-e/replaces:/,$d' <../otelcorecol/builder-config.yaml > internal/config/default.yaml
/Library/Developer/CommandLineTools/usr/bin/make -C cmd/builder ocb
GO111MODULE=on CGO_ENABLED=0 go build -trimpath -o ../../bin/ocb_darwin_arm64 .
$ ./bin/ocb_darwin_arm64
2022-07-26T16:05:06.722+1000	INFO	internal/command.go:91	OpenTelemetry Collector Builder	{"version": "dev", "date": "unknown"}
2022-07-26T16:05:06.722+1000	INFO	internal/command.go:102	Using default build configuration
2022-07-26T16:05:06.723+1000	INFO	builder/config.go:99	Using go	{"go-executable": "/opt/homebrew/bin/go"}
2022-07-26T16:05:06.724+1000	INFO	builder/main.go:76	Sources created	{"path": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1809706932"}
2022-07-26T16:05:06.913+1000	INFO	builder/main.go:108	Getting go modules
2022-07-26T16:05:06.942+1000	INFO	builder/main.go:87	Compiling
2022-07-26T16:05:07.948+1000	INFO	builder/main.go:94	Compiled	{"binary": "/var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/otelcol-distribution1809706932/otelcorecol"}

Documentation:

None.

codecov · 2022-07-26T06:09:30Z

Codecov Report

Base: 91.89% // Head: 91.90% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (17f982e) compared to base (36f0b5c).
Patch has no changes to coverable lines.

❗ Current head 17f982e differs from pull request most recent head b52d1b5. Consider uploading reports for the commit b52d1b5 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5752      +/-   ##
==========================================
+ Coverage   91.89%   91.90%   +0.01%     
==========================================
  Files         213      200      -13     
  Lines       13357    12414     -943     
==========================================
- Hits        12274    11409     -865     
+ Misses        862      793      -69     
+ Partials      221      212       -9

Impacted Files	Coverage Δ
extension/experimental/storage/nop_client.go	`33.33% <0.00%> (-16.67%)`	⬇️
component/extension.go	`86.66% <0.00%> (-13.34%)`	⬇️
pdata/plog/json.go	`85.00% <0.00%> (-13.06%)`	⬇️
pdata/internal/data/bytesid.go	`81.25% <0.00%> (-6.75%)`	⬇️
processor/processorhelper/logs.go	`84.37% <0.00%> (-5.63%)`	⬇️
processor/processorhelper/traces.go	`84.37% <0.00%> (-5.63%)`	⬇️
processor/processorhelper/metrics.go	`84.37% <0.00%> (-5.63%)`	⬇️
exporter/exporterhelper/traces.go	`83.87% <0.00%> (-2.80%)`	⬇️
exporter/exporterhelper/logs.go	`81.96% <0.00%> (-2.78%)`	⬇️
exporter/exporterhelper/metrics.go	`81.96% <0.00%> (-2.78%)`	⬇️
... and 57 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

TylerHelmuth

Is it possible to add a new happy-path test to test/test.sh to test this new functionality? It would be good to have a test that verifies the ocb can be run without a config.

cmd/builder/internal/config/default.yaml

TylerHelmuth · 2022-07-26T15:08:22Z

cmd/builder/internal/config/default.go

+// configuration file. This is the same configuration that otelcorecol is
+// built with.
+func DefaultProvider() koanf.Provider {
+	return fs.Provider(configs, "default.yaml")


Do we need to use FS here or could we use file.Provider instead? I ask bc Fs is listed as experimental still.

Where is it marked experimental?

https://pkg.go.dev/github.com/knadh/koanf@v1.4.2/providers/fs

I found it marked experimental on the github page: https://github.com/knadh/koanf#bundled-providers. That particular line hasn't been updated in 13 months, so could be out of date.

I see. I was looking at the source and godoc, which doesn't mention experimental status. We can switch to the file provider, but the code needed for that will be basically the same as the fs one, so I don't think there's much benefit.

jpeach · 2022-07-27T01:22:43Z

Is it possible to add a new happy-path test to test/test.sh to test this new functionality? It would be good to have a test that verifies the ocb can be run without a config.

Done

builder jpeach$ ./test/test.sh
Using /opt/homebrew/bin/go to compile the distributions.
Running the tests in /var/folders/hf/8frlwk6s40j0wj3jpgbq94z00000gp/T/tmp.1yTemUe3I7
✅ PASS default
✅ PASS core

jpeach · 2022-07-27T02:44:58Z

Pretty sure that this PR doesn't break contrib-tests.

jpkrohling · 2022-08-01T14:53:20Z

I just restarted the failed job. If it fails, please try rebasing this PR, as there might be some incompatibility problem not caused by this PR but fixed in the main branch already (on either side).

jpkrohling · 2022-08-01T17:23:55Z

PR rebased.

jpeach · 2022-08-02T06:45:35Z

# github.com/influxdata/influxdb-observability/common
Error: /home/runner/go/pkg/mod/github.com/influxdata/influxdb-observability/common@v0.2.24/metrics_sort.go:56:43: hdp.MExplicitBounds undefined (type internal.HistogramDataPoint has no field or method MExplicitBounds)
Error: /home/runner/go/pkg/mod/github.com/influxdata/influxdb-observability/common@v0.2.24/metrics_sort.go:57:21: hdp.MExplicitBounds undefined (type internal.HistogramDataPoint has no field or method MExplicitBounds)

jpkrohling · 2022-08-02T16:50:00Z

The failure is related to #5770

jpkrohling

LGTM, but the versions need to be bumped.

cmd/builder/internal/config/default.yaml

jpeach · 2022-08-09T03:15:09Z

LGTM, but the versions need to be bumped.

Updated

bogdandrutu · 2022-08-12T20:26:29Z

CHANGELOG.md

 - `ocb` now exits with an error if it fails to load the build configuration. (#5731)
- Deprecate `HTTPClientSettings.ToClientWithHost` (#5737)
+- Deprecate `HTTPClientSettings.ToClientWithHost`. (#5737)


Confused about these changes...

Just making the formatting consistent

Please split them in a separate PR :)

reverted and rebased

I can rebase this again. Is it ready to merge when I do?

rebases 🤷

@bogdandrutu, I believe this PR is pending on your side. Would you be able to review/merge this?

I think it should be your side as well, we should not have "refactoring, formatting" changes in the same PR. I sent a separate PR #6012

bogdandrutu · 2022-09-01T18:27:38Z

@jpkrohling can you document the motivation? Does not seem to be that useful to produce a standard collector that we already produce and release anyway?

jpeach · 2022-09-01T21:55:45Z

@jpkrohling can you document the motivation? Does not seem to be that useful to produce a standard collector that we already produce and release anyway?

Originally, when you ran ocb with no configuration, it generated a collector with no plugins, which was completely useless (see my other PR which adds a way to know what plugins you have included in the collector). The original documentation for all this claimed that ocb would produce a "default collector" when run without any arguments, and this PR restores that claim, so users can build the default collector.

Embed the build configuration that is used to build otelcorecol into ocb so that end users can easily generate a useful collector. This makes the `--config` flag optional again. Signed-off-by: James Peach <jpeach@cloudflare.com>

jpeach · 2022-09-11T23:07:11Z

@open-telemetry/collector-approvers is this ready to merge?

jpkrohling

LGTM, but I can't merge.

@bogdandrutu, could you review and merge this if you think it's ready?

jpeach requested review from a team and dmitryax July 26, 2022 06:05

jpeach force-pushed the ocb-default-config branch from 1034358 to a390ad2 Compare July 26, 2022 06:06

TylerHelmuth reviewed Jul 26, 2022

View reviewed changes

jpeach force-pushed the ocb-default-config branch from a390ad2 to d53e564 Compare July 27, 2022 01:22

jpkrohling self-requested a review July 28, 2022 17:31

jpkrohling force-pushed the ocb-default-config branch from d53e564 to 30bfaa0 Compare August 1, 2022 17:23

jpeach force-pushed the ocb-default-config branch from 30bfaa0 to 74a3f94 Compare August 2, 2022 05:46

jpeach force-pushed the ocb-default-config branch 2 times, most recently from e9e3c44 to 08f3c67 Compare August 3, 2022 00:17

TylerHelmuth approved these changes Aug 3, 2022

View reviewed changes

jpkrohling reviewed Aug 8, 2022

View reviewed changes

cmd/builder/internal/config/default.yaml Outdated Show resolved Hide resolved

jpeach force-pushed the ocb-default-config branch from 08f3c67 to 6fe7145 Compare August 9, 2022 03:13

jpeach force-pushed the ocb-default-config branch from 6fe7145 to dfc3d9e Compare August 9, 2022 03:19

jpkrohling approved these changes Aug 10, 2022

View reviewed changes

bogdandrutu reviewed Aug 12, 2022

View reviewed changes

jpeach force-pushed the ocb-default-config branch from 4443611 to 5eeee8b Compare August 16, 2022 02:51

jpeach force-pushed the ocb-default-config branch from 17f982e to 9a7dcfd Compare August 25, 2022 01:19

Add a default build configuration to ocb.

b52d1b5

Embed the build configuration that is used to build otelcorecol into ocb so that end users can easily generate a useful collector. This makes the `--config` flag optional again. Signed-off-by: James Peach <jpeach@cloudflare.com>

jpeach force-pushed the ocb-default-config branch from 9a7dcfd to b52d1b5 Compare September 11, 2022 23:01

jpkrohling approved these changes Sep 12, 2022

View reviewed changes

bogdandrutu merged commit 9f14f5e into open-telemetry:main Sep 13, 2022

This was referenced Sep 14, 2022

[docs] Update release instructions #6068

Merged

[CI] Builder integration tests fail on release prep #6076

Closed

jpeach deleted the ocb-default-config branch September 14, 2022 22:52

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

Add a default build configuration to ocb. #5752

Add a default build configuration to ocb. #5752

jpeach commented Jul 26, 2022

codecov bot commented Jul 26, 2022 •

edited

Loading

TylerHelmuth left a comment

TylerHelmuth Jul 26, 2022

jpeach Jul 27, 2022

TylerHelmuth Jul 27, 2022

jpeach Jul 27, 2022

jpeach commented Jul 27, 2022

jpeach commented Jul 27, 2022

jpkrohling commented Aug 1, 2022 •

edited

Loading

jpkrohling commented Aug 1, 2022

jpeach commented Aug 2, 2022

jpkrohling commented Aug 2, 2022

jpkrohling left a comment

jpeach commented Aug 9, 2022

bogdandrutu Aug 12, 2022

jpeach Aug 15, 2022

bogdandrutu Aug 15, 2022

jpeach Aug 16, 2022

jpeach Aug 23, 2022

jpeach Aug 25, 2022

jpkrohling Aug 25, 2022

bogdandrutu Sep 1, 2022 •

edited

Loading

bogdandrutu commented Sep 1, 2022

jpeach commented Sep 1, 2022

jpeach commented Sep 11, 2022

jpkrohling left a comment

Add a default build configuration to ocb. #5752

Add a default build configuration to ocb. #5752

Conversation

jpeach commented Jul 26, 2022

codecov bot commented Jul 26, 2022 • edited Loading

Codecov Report

TylerHelmuth left a 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

jpeach commented Jul 27, 2022

jpeach commented Jul 27, 2022

jpkrohling commented Aug 1, 2022 • edited Loading

jpkrohling commented Aug 1, 2022

jpeach commented Aug 2, 2022

jpkrohling commented Aug 2, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

jpeach commented Aug 9, 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

Choose a reason for hiding this comment

bogdandrutu Sep 1, 2022 • edited Loading

Choose a reason for hiding this comment

bogdandrutu commented Sep 1, 2022

jpeach commented Sep 1, 2022

jpeach commented Sep 11, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 26, 2022 •

edited

Loading

jpkrohling commented Aug 1, 2022 •

edited

Loading

bogdandrutu Sep 1, 2022 •

edited

Loading