Some issues with enums management in API

[jotak]

Problem description

I'm seeing several issues with the current way enums are managed in the api, making them unpractical to work with:

• frequent confusion in how to refer to enum: as their yaml key, or upper camel case? both are used depending on the context
• lack of type enforcement: enums are defined as string.
• multiplicity of definitions: enum values are set in different places (enum struct + api.go const), developers need to make sure these defs are always sync'ed; plus, they are sometimes referred to as CamelCase strings equivalent.
• some boilerplate to add when a new enum field is created: a conversion function + a declaration in enum.go

Example of usage that I find confusing:

What doc says:

         balancer: (enum) one of the following:
             roundRobin: RoundRobin balancer
             leastBytes: LeastBytes balancer
             hash: Hash balancer
             crc32: Crc32 balancer
             murmur2: Murmur2 balancer

How enum is defined:

type KafkaEncodeBalancerEnum struct {
	RoundRobin string `yaml:"roundRobin" json:"roundRobin" doc:"RoundRobin balancer"`
	LeastBytes string `yaml:"leastBytes" json:"leastBytes" doc:"LeastBytes balancer"`
	Hash       string `yaml:"hash" json:"hash" doc:"Hash balancer"`
	Crc32      string `yaml:"crc32" json:"crc32" doc:"Crc32 balancer"`
	Murmur2    string `yaml:"murmur2" json:"murmur2" doc:"Murmur2 balancer"`
}

This structure is sort of a hack to allow documentation and also to perform conversions via reflection. It isn't used to carry data.

Usage in yaml:

      kafka:
        address: 1.2.3.4:9092
        topic: topic1
          balancer: roundRobin

Usage in go code:

	var balancer kafkago.Balancer
	switch config.Balancer {
	case api.KafkaEncodeBalancerName("RoundRobin"):
		balancer = &kafkago.RoundRobin{}
	case api.KafkaEncodeBalancerName("LeastBytes"):
		balancer = &kafkago.LeastBytes{}
	case api.KafkaEncodeBalancerName("Hash"):
		balancer = &kafkago.Hash{}
	case api.KafkaEncodeBalancerName("Crc32"):
		balancer = &kafkago.CRC32Balancer{}
	case api.KafkaEncodeBalancerName("Murmur2"):
		balancer = &kafkago.Murmur2Balancer{}
	default:
		balancer = nil
	}

It's the usage in go code that I find very confusing: it doesn't use the documented format in strings, but uses upper camel-case instead. Moreover, it lacks of type enforcement, as it accepts any string in the switch statement, making it more bug-prone if a wrong value is provided, or in case of typo, etc.

Proposal

Instead of all of this, I wish we had just one reference point to declare enums, and make it as a type alias for better type enforcement:

type MetricFilterEnum string

const (
	MetricFilterEqual MetricFilterEnum = "equal"
	MetricFilterNotEqual MetricFilterEnum = "not_equal"
	MetricFilterPresence MetricFilterEnum = "presence"
	MetricFilterAbsence MetricFilterEnum = "absence"
	MetricFilterRegex MetricFilterEnum = "match_regex"
	MetricFilterNotRegex MetricFilterEnum = "not_match_regex"
)

type MetricsFilter struct {
         # ...
	Type  MetricFilterEnum `yaml:"type" json:"type" doc:"the type of filter match (enum)"`
}

We would remove:

• enum values as constant definitions in api.go
• enum reference in enum.go
• the enum struct that is only there to carry doc & perform conversions by reflection
• the conversion function

The downside is that we'd loose the enum values documentation. We cannot have doc markers on constants.

An option could be to use go documentation, and add some logic to the docgen scripts to use go doc to extract this documentation and inject it in the api doc file:

The go constants would be documented as such:

const (
	// match exactly the provided filter value
	MetricFilterEqual MetricFilterEnum = "equal"
	// the value must be different from the provided filter
	MetricFilterNotEqual MetricFilterEnum = "not_equal"
	// filter key must be present (filter value is ignored)
	MetricFilterPresence MetricFilterEnum = "presence"
	// filter key must be absent (filter value is ignored)
	MetricFilterAbsence MetricFilterEnum = "absence"
	// match filter value as a regular expression
	MetricFilterRegex MetricFilterEnum = "match_regex"
	// the filter value must not match the provided regular expression
	MetricFilterNotRegex MetricFilterEnum = "not_match_regex"
)

Then with go doc -all -short -C pkg/api MetricFilterEnum and some sed magic, this could be reinjected into docs/api.md.

This is not as pretty as the current implementation in terms of doc generation, but IMO we should favor code quality over this.

[github-actions]

Congratulations for contributing your first flowlogs-pipeline issue

[jotak]

Hey @KalmanMeth @OlivierCazade @jpinsonneau , any thoughts to share on this? Personally I'd like to go forward for the gains it would provide (better type safety / less duplication etc.) and do the changes but I recognize there's a drawback especially wrt. the doc tag on enum that needs to be done differently

You can check this PR to see how it'd look like

[KalmanMeth]

@eranra Your input would also be appreciated.

[jpinsonneau]

I'm fine going that direction as soon as the doc is kept 👍
Let's just ensure to refactor all at once to avoid keeping both approach

[KalmanMeth]

I agree that some of the enums are not really used in the coding and it is confusing and error-prone for the programmer. At the same time we need to clearly define the legal values used for the various parameters. I think we should strive to make the use of the enums uniform across the various stages, and at the same time include (one way or another) the possible values in the documentation. If we have a decent solution on how to document the parameter values directly from the code, then I am OK with the proposed changes.

[jotak]

thanks @KalmanMeth @jpinsonneau
I updated the current PR to refactor all at once as Julien suggested. @KalmanMeth , the current documentation is fully preserved - it's just that the doc gen script is less straightforward to understand, but in the end it's still just about running make doc and the output still lists all enum values