Add sub command to generate sample configuration for the collector #5943

neelayu · 2022-08-22T14:14:18Z

Description:
Adding a feature - Create a subcommand that generates a sample configuration for the collector. It has support for UI based options(like npm init), or via flags.

Link to tracking Issue: #5654

Testing: Unit Tests

Documentation: Need suggestions

Waiting for suggestions to improve the code more and opinions on how we can improve the documentation.

Sample workflow:

otelcorecol git:(config-sub) ✗ go run . config --help
Print sample configuration

Usage:
  otelcorecol config [flags]

Flags:
  -f, --config-file string      Name of the config file (default "otelcol.config.yaml")
  -d, --dry-run                 Dry run
  -e, --exporter stringArray    Exporters to be configured
  -x, --extension stringArray   Extensions to be configured
  -h, --help                    help for config
  -l, --pipeline stringArray    Specify the pipelines to be configured
  -p, --processor stringArray   Processors to be configured
  -r, --receiver stringArray    Receivers to be configured

If no flags are provided, use prompts
It uses github.com/AlecAivazis/survey go library.

With flags

Output(only some parts for relevance)

exporters:
  logging:
    # LogLevel defines log level of the logging exporter; options are debug, info, warn, error.
    loglevel: 0
    # SamplingInitial defines how many samples are initially logged during each second.
    sampling_initial: 2
    # SamplingThereafter defines the sampling rate after the initial samples are logged.
    sampling_thereafter: 500
  otlp:
     ...

 ...
service:
  ..

As you can see, comments are also added for better understanding. This is achieved by github.com/open-telemetry/opentelemetry-collector-contrib/cmd/configschema

Suggestions are welcome to improve this PR.
Thanks!

codecov · 2022-08-22T14:20:28Z

Codecov Report

Base: 91.76% // Head: 91.93% // Increases project coverage by +0.16% 🎉

Coverage data is based on head (5995f34) compared to base (e981169).
Patch coverage: 94.84% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5943      +/-   ##
==========================================
+ Coverage   91.76%   91.93%   +0.16%     
==========================================
  Files         217      218       +1     
  Lines       13350    13770     +420     
==========================================
+ Hits        12251    12659     +408     
- Misses        870      875       +5     
- Partials      229      236       +7

Impacted Files	Coverage Δ
service/configinit.go	`94.11% <94.11%> (ø)`
component/component.go	`100.00% <100.00%> (ø)`
component/exporter.go	`100.00% <100.00%> (ø)`
component/extension.go	`100.00% <100.00%> (ø)`
component/processor.go	`100.00% <100.00%> (ø)`
component/receiver.go	`100.00% <100.00%> (ø)`
exporter/loggingexporter/factory.go	`100.00% <100.00%> (ø)`
exporter/otlpexporter/factory.go	`91.04% <100.00%> (+0.13%)`	⬆️
exporter/otlphttpexporter/factory.go	`87.09% <100.00%> (+0.14%)`	⬆️
extension/ballastextension/factory.go	`100.00% <100.00%> (ø)`
... and 10 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.

svrnm · 2022-08-23T10:13:55Z

Hey @neelayu, thanks for creating this. That's much more than I expected when I raised #5654!

I did not review the code, leaving that to the @open-telemetry/collector-approvers, but I tried out the functionality:

I like the prompts! Two issues:

If I skip all prompts I get the following YAML:

exporters: {}
processors: {}
receivers: {}
extensions: {}
service:
  # Specify extensions for the collector
  extensions: []
  pipelines: {}

Is it possible to not have {} for an empty set?

If I select all/some attributes it fails with

2022/08/23 12:03:16 open ../../go.mod: no such file or directory

This seems to be a hard-coded file path, when I copy my compiled collector binary into a folder like "./bin/bin" within the forked repository things work. You may need to verify that the final collector works independent of the existence of these files.

To mention that: I like that if I only provide the -f <filename> I get an empty file and not the "missing" prompts. This is really helpful if I just need an empty config that I might like to edit in a second step.

Regarding the copied comments: On the one hand I like them, on the other hand they can make a config extremely hard to read for a new-comer, e.g. otelcol config -p batch -e otlp -d creates a config with 106 lines. Maybe you can provide another flag that enables/disables to fill out the config with all the attributes + comments?

neelayu · 2022-08-23T11:11:22Z

@svrnm Thanks for going through this PR and giving it a try.

If I skip all prompts I get the following YAML:
yaml code
Is it possible to not have {} for an empty set?

Yes. This is something which I want an opinion on. I'd like to add validations to check if the inputs were provided or not.

If I select all/some attributes it fails with

2022/08/23 12:03:16 open ../../go.mod: no such file or directory
Correct. I have to fix this!

on the other hand they can make a config extremely hard to read for a new-comer,
They do make the configuration file pretty large. As you mentioned, are these options better?

otelcol config -p batch -e otlp --no-comments

Will produce all fields with no comments.

otelcol config -p batch -e otlp --no-attrs

Will produce only the configuration file with key headers?

exporters:
  otlp:
     # For configuration params, refer https://pkg.go.dev/go.opentelemetry.io/collector/exporter/otlpexporter

As you can see, there are no attributes, but a single line comment indicating where the info on the configuration can be found. I believe pkg.go.dev will do the job.

I think its difficult to get only "mandatory" or "minimal" configuration fields from the struct. So its either all or none.

svrnm · 2022-08-23T15:19:15Z

@svrnm Thanks for going through this PR and giving it a try.

If I skip all prompts I get the following YAML:
yaml code
Is it possible to not have {} for an empty set?

Yes. This is something which I want an opinion on. I'd like to add validations to check if the inputs were provided or not.

I personally think it's ok to leave them empty.

on the other hand they can make a config extremely hard to read for a new-comer,
They do make the configuration file pretty large. As you mentioned, are these options better?
otelcol config -p batch -e otlp --no-comments
Will produce all fields with no comments.
otelcol config -p batch -e otlp --no-attrs
Will produce only the configuration file with key headers?
exporters:
  otlp:
     # For configuration params, refer https://pkg.go.dev/go.opentelemetry.io/collector/exporter/otlpexporter

I like that, teaches people also where they can look for details!

I am only wondering what should be the default (full details, only attributes, no comments&no attributes)?

As you can see, there are no attributes, but a single line comment indicating where the info on the configuration can be found. I believe pkg.go.dev will do the job.

I think its difficult to get only "mandatory" or "minimal" configuration fields from the struct. So its either all or none.

That's an interesting one! What about something like:

otelcol config -p batch -e otlp,endpoint=https://mybackend/,entpoint.headers.apiKey=asdf,tls.insecure=true --no-attrs

gives you some of those attributes set (and skips the rest)?

neelayu · 2022-08-26T13:52:47Z

Hey @svrnm I have made a few changes. I didnt really get time this week to work on it.
Re:

That's an interesting one! What about something like:

otelcol config -p batch -e otlp,endpoint=https://mybackend/,entpoint.headers.apiKey=asdf,tls.insecure=true --no-attrs

gives you some of those attributes set (and skips the rest)?

I believe this is as good as knowing what params are present. So the user might as well use that and create a config file.

svrnm · 2022-08-29T08:44:26Z

@neelayu thank you, I am looking forward to have this feature. I am eventually just a consumer of that, as an end-user but also to simplify the Getting Started docs for the collector over at opentelemetry.io :)

jpkrohling

I left a few comments, and while I think this feature would be useful, I think the current implementation might need some more attention. I ran the simplest command possible, $ go run ./ config -e otlp, and I got a file that is full of options that are not needed while still not being very functional, given that the collector won't start due to missing values for required properties (endpoint, for instance).

Instead of having one command for all possible components, how about having an interface for components to implement? This way, component developers can decide what and how to ask users, potentially even running the validation in the process.

Concretely, OTLP Exporter would only ask for the "endpoint" property and ask users if they want to specify values for optional properties.

service/configinit_test.go

service/configinit.go

linux-foundation-easycla · 2022-09-05T15:51:08Z

The committers listed above are authorized under a signed CLA.

✅ login: neelayu / name: Neelay Upadhyaya (aecaaf2, b9323b4, caf719f, b6b41af, 044642b, fbd463c, 2a1b2b2, 1244b33, e0186cc)

neelayu · 2022-09-05T16:12:56Z

I have refactored the code. The basefactory essentially adds a SampleConfig() method. The authors are expected to return a configuration for the component. This is more or less what Telegraf agent does in its approach. Since I didnt want to introduce breaking changes across the contrib repo as well, I have added it as an option. Let me know if this approach works. @jpkrohling

github-actions · 2022-10-12T03:56:06Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

svrnm · 2022-10-12T06:47:25Z

I am still a big fan of this and keen to see this happening, is there anything I can do to help this progressing? ty

mx-psi · 2022-10-13T12:07:14Z

@jpkrohling is there anything needed here?

Aneurysm9

Something about having all of the *.sample.yaml files added here doesn't feel right to me. It seems like the structure of these files is dictated by the structure of the types they represent. The addition here is documentary comments and sample values. I wonder whether this could be accomplished with well-structured godoc comments that can be accessed with the reflection API. Doing so would avoid repeating information for types that are commonly embedded in other types and also minimize the risk of changes to the meaning, interpretation, or documentation of a field not being correctly propagated to all sample configurations where it appears.

neelayu · 2022-10-18T11:24:47Z

@Aneurysm9 I did not add *sample.yaml in my first few drafts. You can check the approach which I used here 044642b

In that I used the configschema API from contrib repo to fetch the fields and doc info. The limitation of that approach is that we can't obtain sensible defaults for fields which are mandatory.(Eg endpoint)

jpkrohling

I like where this is going, but I believe that:

this needs a bit better code organization, as there's just too much happening on the configinit.go file
if the sample YAML files are auto-generated, there should be a couple of make targets to generate the files on demand and CI checks to ensure that those files are not changed (or are changed when the base type changes). I believe a header stating that those files are auto-generated would also be warranted

jpkrohling · 2022-10-19T00:35:38Z

component/component.go

 	unexportedFactoryFunc()
 }

 type baseFactory struct {
-	cfgType config.Type
+	cfgType      config.Type
+	sampleConfig string
 }

 func (baseFactory) unexportedFactoryFunc() {}


I believe this won't cause backward compatibility issues because we are forcing implementations to embed the base factory, right?

jpkrohling · 2022-10-19T00:37:56Z

service/configinit.go

+)
+
+const (
+	sampleConfigHeader = `Opentelemetry Collector sample configuration. Double check the pipeline configuration


Suggested change

sampleConfigHeader = `Opentelemetry Collector sample configuration. Double check the pipeline configuration

sampleConfigHeader = `OpenTelemetry Collector sample configuration. Double-check the pipeline configuration

jpkrohling · 2022-10-19T00:39:10Z

service/configinit.go

@@ -0,0 +1,546 @@
+// Copyright The OpenTelemetry Authors


Not quite sure I agree this is the best package for this code.

jpkrohling · 2022-10-19T00:40:30Z

service/configinit.go

+var log *zap.Logger
+var opts = new(optionsSelected)
+
+func init() {


I don't see a strong reason to use an init function here. Perhaps name it differently and invoke it where the logger needs to be set?

jpkrohling · 2022-10-19T00:42:55Z

service/configinit.go

+				}
+			}
+
+			exportersNode, err := getExportersNode(set.Factories, opts.Exporters)


Could you split the business from the plumbing? Plumbing would be setting up the logger, validating the user input, returning errors, printing the outcome, and so on. Assembling the list of components and their sample config would be the business.

neelayu · 2022-10-19T06:05:26Z

Thanks @jpkrohling for the comments. I just want to ensure that this approach of sample config yaml makes sense. One other approach I can think of creating a web equivalent config creation which was discussed in SIG meeting in CLI. That is for every field we ask the user to input the value, the docstring will act as help text.
Along with it, we can have an option of --full which will create the configuration with no user input, the one which I created in my first commit.
Let me know which approach we want to take.

If we stick to sample yaml approach, I am aware that the yaml files will have to be created via make targets. However, that would add too much code to review in this PR. So I can take it up in a separate task. Although there is one caveat here, the files will be auto-generated, but they would be bulky with many fields and large comment sections. So the onus lies on the developer(author of the component) to select(or not) which ones are relevant for a sample config.

github-actions · 2022-11-03T03:40:31Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jpkrohling · 2022-11-07T15:04:27Z

Sorry, I need to catch-up with the collector's current state before giving any meaningful input here. Perhaps @codeboten knows what's the state around this part, especially regarding the UI that was shown some weeks ago to help with the collector's config?

github-actions · 2022-11-23T03:23:34Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-12-08T03:16:17Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-12-23T03:15:55Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

neelayu requested review from a team and dmitryax August 22, 2022 14:14

neelayu force-pushed the config-sub branch from c04b86f to 044642b Compare August 26, 2022 13:50

neelayu mentioned this pull request Aug 26, 2022

Improve doc comments on configuration structs and fields open-telemetry/opentelemetry-collector-contrib#13384

Closed

jpkrohling reviewed Aug 29, 2022

View reviewed changes

neelayu force-pushed the config-sub branch from df926e9 to e0186cc Compare September 5, 2022 16:05

neelayu force-pushed the config-sub branch from e0186cc to 533b304 Compare September 7, 2022 09:01

neelayu marked this pull request as draft September 7, 2022 09:45

jpkrohling mentioned this pull request Sep 9, 2022

Create a lightweight web app to create a collector config open-telemetry/opentelemetry-collector-contrib#13991

Closed

neelayu force-pushed the config-sub branch from 533b304 to b803719 Compare September 13, 2022 11:03

neelayu marked this pull request as ready for review September 13, 2022 11:07

neelayu force-pushed the config-sub branch from 4ff88e1 to 67473c6 Compare September 16, 2022 06:29

neelayu added 5 commits September 27, 2022 14:09

Add sub command to generate sample configuration for the collector

609515c

Fix flaky test

bde45cc

lint fix

521cb17

test fix for windows

4d476bc

rebase and add unit tests

5995f34

neelayu force-pushed the config-sub branch from 67473c6 to 5995f34 Compare September 27, 2022 11:06

github-actions bot added the Stale label Oct 12, 2022

dmitryax removed the Stale label Oct 12, 2022

Aneurysm9 reviewed Oct 17, 2022

View reviewed changes

jpkrohling reviewed Oct 19, 2022

View reviewed changes

github-actions bot added the Stale label Nov 3, 2022

github-actions bot removed the Stale label Nov 8, 2022

github-actions bot added the Stale label Nov 23, 2022

jpkrohling removed the Stale label Nov 23, 2022

github-actions bot added the Stale label Dec 8, 2022

github-actions bot closed this Dec 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add sub command to generate sample configuration for the collector #5943

Add sub command to generate sample configuration for the collector #5943

neelayu commented Aug 22, 2022 •

edited

Loading

codecov bot commented Aug 22, 2022 •

edited

Loading

svrnm commented Aug 23, 2022

neelayu commented Aug 23, 2022

svrnm commented Aug 23, 2022

neelayu commented Aug 26, 2022

svrnm commented Aug 29, 2022

jpkrohling left a comment

linux-foundation-easycla bot commented Sep 5, 2022 •

edited

Loading

neelayu commented Sep 5, 2022

github-actions bot commented Oct 12, 2022

svrnm commented Oct 12, 2022

mx-psi commented Oct 13, 2022

Aneurysm9 left a comment

neelayu commented Oct 18, 2022

jpkrohling left a comment

jpkrohling Oct 19, 2022

jpkrohling Oct 19, 2022

jpkrohling Oct 19, 2022

jpkrohling Oct 19, 2022

jpkrohling Oct 19, 2022

neelayu commented Oct 19, 2022

github-actions bot commented Nov 3, 2022

jpkrohling commented Nov 7, 2022

github-actions bot commented Nov 23, 2022

github-actions bot commented Dec 8, 2022

github-actions bot commented Dec 23, 2022

	sampleConfigHeader = `Opentelemetry Collector sample configuration. Double check the pipeline configuration
	sampleConfigHeader = `OpenTelemetry Collector sample configuration. Double-check the pipeline configuration

Add sub command to generate sample configuration for the collector #5943

Add sub command to generate sample configuration for the collector #5943

Conversation

neelayu commented Aug 22, 2022 • edited Loading

codecov bot commented Aug 22, 2022 • edited Loading

Codecov Report

svrnm commented Aug 23, 2022

neelayu commented Aug 23, 2022

svrnm commented Aug 23, 2022

neelayu commented Aug 26, 2022

svrnm commented Aug 29, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

linux-foundation-easycla bot commented Sep 5, 2022 • edited Loading

neelayu commented Sep 5, 2022

github-actions bot commented Oct 12, 2022

svrnm commented Oct 12, 2022

mx-psi commented Oct 13, 2022

Aneurysm9 left a comment

Choose a reason for hiding this comment

neelayu commented Oct 18, 2022

jpkrohling left a comment

Choose a reason for hiding this comment

jpkrohling Oct 19, 2022

Choose a reason for hiding this comment

jpkrohling Oct 19, 2022

Choose a reason for hiding this comment

jpkrohling Oct 19, 2022

Choose a reason for hiding this comment

jpkrohling Oct 19, 2022

Choose a reason for hiding this comment

jpkrohling Oct 19, 2022

Choose a reason for hiding this comment

neelayu commented Oct 19, 2022

github-actions bot commented Nov 3, 2022

jpkrohling commented Nov 7, 2022

github-actions bot commented Nov 23, 2022

github-actions bot commented Dec 8, 2022

github-actions bot commented Dec 23, 2022

neelayu commented Aug 22, 2022 •

edited

Loading

codecov bot commented Aug 22, 2022 •

edited

Loading

linux-foundation-easycla bot commented Sep 5, 2022 •

edited

Loading