Skip to content
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

Allow running service.Collector without CLI interface #3957

Closed
4 tasks done
mx-psi opened this issue Sep 1, 2021 · 8 comments
Closed
4 tasks done

Allow running service.Collector without CLI interface #3957

mx-psi opened this issue Sep 1, 2021 · 8 comments
Assignees
Labels
area:service enhancement New feature or request

Comments

@mx-psi
Copy link
Member

mx-psi commented Sep 1, 2021

Is your feature request related to a problem? Please describe.

The public API of service.Collector allows for the creation of different distros easily, which can customize the components included, logging, and configuration among other aspects, and it provides a common CLI through the Command method. However, there is no way to customize or disable the CLI interface or customize the values of the CLI flags exposed here

// TODO: coalesce this code and expose this information to other components.
flagSet := new(flag.FlagSet)
addFlagsFns := []func(*flag.FlagSet){
configtelemetry.Flags,
parserprovider.Flags,
telemetry.Flags,
builder.Flags,
loggerFlags,
}

This makes it harder to embed the Collector in an existing binary application and expose its functionality in an uniform way.

Describe the solution you'd like

The Collector (or some other intermediate public struct) allows setting all configuration through either the config parser or a configuration struct like service.CollectorSettings, while keeping the existing option to parse CLI flags as a separate step. This would involve (at least) the following flags parsing to be refactored:

Describe alternatives you've considered

You can use the Run method and use the default values for the non-exposed configuration.

Additional context

This relates broadly to the goals on #3482.

@alolita alolita added area:service enhancement New feature or request labels Sep 2, 2021
@bogdandrutu
Copy link
Member

Related to #3843

@bogdandrutu
Copy link
Member

@mx-psi the configuration related flags, are just flags that allow to overwrite any property in the config, so they cannot and don't have to be removed since obviously everything can be configured in the config :)

@mx-psi
Copy link
Member Author

mx-psi commented Sep 10, 2021

@bogdandrutu My point with this issue is that the CLI code is not separate from the rest of the code. Right now, if you call Collector.Run it will eventually parse os.Args[1:], which you may not want to do if you are adding a Collector inside another application that already has a CLI.

For some of the flags above, the solution is to remove them, but for the ones that can't be removed, the solution is to provide a way to not run the CLI parsing. Does that make sense? Is that a use case that the Collector should support?

@bogdandrutu
Copy link
Member

@mx-psi tell me more how do you envision to support the second part?

@mx-psi
Copy link
Member Author

mx-psi commented Sep 10, 2021

@bogdandrutu One way to support that would be to have an intermediate struct, say Application where all the settings that need to be set as flags can be passed around in a config struct of some kind:

type Application struct {
    // same thing as service.Collector now, without the cobra/flags bits
}

type ApplicationSettings struct {
   // all of CollectorSettings fields go here
   // ...
   OverrideFlags map[string]interface{} // the --set flag value
   MetricsLevel configtelemetry.Level  // the --metrics-level value 
   // ... other flag values
}

and then the Collector struct would be rewritten to just parse the CLI flags and build an Application passing the flag values through ApplicationSettings.

Maybe the configuration flags in particular do not even have to be in ApplicationSettings but just be handled by the function that builds theCollector (since they are really not Collector flags, but flags for the default parser provider).

@alolita
Copy link
Member

alolita commented Sep 15, 2021

@bogdandrutu are there other PRs related to this issue in the pipeline?

@alolita
Copy link
Member

alolita commented Sep 16, 2021

@bogdandrutu is this issue done? :-)

@bogdandrutu
Copy link
Member

@alolita see tracking items in the issue description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:service enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants