Add --opamp-config flag

[echlebek]

The --opamp-config flag is used to specify an OpAmp configuration file. The file will be updated according to the OpAmp server's remote management directives.

Since this method of configuration is so different from the configuration expressed by the --config flag, --opamp-config and --config cannot be combined.

This change marks the second time it was necessary to patch main.go, and so I've created an adhoc patch queue by prefixing patches with numbers that indicate the order the patches should be applied in.

This change is part of an epic to add OpAmp remote management to otelcol-sumo: https://sumologic.atlassian.net/browse/SUMO-206529

Manual testing performed:

• collector runs with --config (against the example configuration in upstream's repo)
• collector runs with --op-amp config and reports a not-implemented error
• collector refuses to run with both --config and --op-amp config and cites a configuration error

[echlebek]

This PR is ready for review, despite the complaints of one of the linters (it is wrong).

[sumo-drosiek]

We should have in-code tests for that feature. Is there any plan for opamp e2e tests or unit tests?

[swiatekm]

Some structural comments, LGTM otherwise.

[swiatekm]

Is this necessary? You're fetching the flag value via flagset.Lookup anyway, so you don't need to explicitly bind it to a variable. And if we delete this line, we can also drop the second patch file completely, simplifying the setup.

[echlebek]

You made me do a double take here so I went back and tested. It's necessary! Without this line, the flag is never registered in the flag set, doesn't appear in the --help docs, and will be rejected by the flag parser if you try to use it.

[swiatekm]

I was thinking of defining the flag directly on the flagset in UseCustomConfigProvider, before parsing command line arguments. Sorry for not being clear about that.

[echlebek]

@swiatekm-sumo Sorry, I still don't understand how this suggestion is supposed to work. My understanding is that the flagset defined in UseCustomConfigProvider is a distinct flagset from what the otel collector is actually using. So even if a flag is added in this scope, it won't be usable.

https://github.com/SumoLogic/sumologic-otel-collector/blob/main/otelcolbuilder/cmd/configprovider.go#L51

Discarding the output of the flagset here seems to indicate that this was an intentional design choice and that we expect the original otel flagset to be used still. Am I missing something?

I did try to add a flag in this context, but since the entry point of the program still uses the otel flag parser, it isn't recognized or displayed in the help output of the program.

[swiatekm]

Yeah, you're right about that. There isn't really a way to simply add a flag to the flagset used by the upstream collector. The current setup was born of a need to add our own configprovider, which unfortunately required parsing flags for config locations. So what happens is we define our own flagset to get these locations, but ignore parse errors and just let the upstream flag parsing logic take care of them. The thought behind this was that we should try to be maximally compatible with the upstream flags.

However, since we're adding a new flag here and want to handle it properly, including help text and error messages, we either need to bite the bullet and bubble-up errors from our own flagset, or do what you've done originally and define the new flag separately.

Thing is, now that I look at it again, your original solution really shouldn't work, or at least the validation shouldn't. You're adding that flag to the upstream flagset, but use our custom flagset for validation. So it should always come up empty when you do a Lookup on it.

The cleanest solution to this problem is probably to just make our own flagset authoritative and add the flag there.

Sorry for the confusion.

[echlebek]

Please try running the collector with --opamp-config defined. You will see that it fails with the not implemented error here: https://github.com/SumoLogic/sumologic-otel-collector/pull/1230/files#diff-ebc450ec46371fd0d3aa5c5e35bdbc4d41ccb7c5b2837b4a9b6e8be4bbf2a9d1R121

[swiatekm]

Okay, I think the current state is the best we can do without redoing the whole command parsing logic. In that case, I'd like to make it clear via comments why the flags are defined this way:

Suggested change

	+ cmd.Flags().StringVarP(&opAmpConfig, "opamp-config", "", "", "path to opamp config file")
	+ // this flag is only defined here so it shows up in the help text
	+ cmd.Flags().StringVarP(&opAmpConfig, "opamp-config", "", "", "path to opamp config file")

Also, is there a reason we can't move this change to the existing patch file? Why do they need to be separate?

[echlebek]

Happy to add that comment. I won't commit the suggestion as that would result in a malformed patch but I'm happy to redo that patch.

I think it's best to keep patches as small as possible, even if that results in a greater number of them, as it's generally easier to deal with small patches against an upstream project. I've maintained a project against an upstream with several dozen patches and my experience has been that large patches are generally more difficult to work with overall.

[swiatekm]

If you feel strongly about keeping it in two patch files, I don't mind. In all honesty, we should propose being able to customize the templates otelcolbuilder uses upstream to avoid the patching. Or just drop the builder altogether and maintain this bootstrap code ourselves.

[echlebek]

@sumo-drosiek We had a lot of uncertainty around the implementation details of this feature so I filed this PR with unimplemented functionality. There will be a followup PR that is more "meat and potatoes" with tests. I filed this PR mostly to see if people would be OK with the way I handled the patching.

[echlebek]

Thanks for your patience on this one, @swiatekm-sumo