Adding more flexibility to describe a feature#7703
Adding more flexibility to describe a feature#7703urso merged 5 commits intoelastic:masterfrom ph:feature/use-details-instead-of-only-stability
Conversation
libbeat/feature/feature.go
Outdated
There was a problem hiding this comment.
I had to make a choice here, I've decided to have a bigger interface for the description of a feature.
I could have created a small interface named 'Filterable' which would only require to have a 'Stability()' method defined.
libbeat/feature/details.go
Outdated
There was a problem hiding this comment.
nit: it's common in go to end interface names with er. E.g. Describer
libbeat/feature/details.go
Outdated
There was a problem hiding this comment.
What do we need String for? Is it supposed to be the short name?
There was a problem hiding this comment.
I've removed but details will still implement the stringer interface.
There was a problem hiding this comment.
I'm a little undecided here if we should rather use a struct instead of an interface+constructor.
Pro:
- Especially with go-plugins I see the advantage of using an interface for the details. One can use custom ones. Even vendoring hickups will be no problem.
- We can provide alternative constructors, e.g. filling in
feature.Stableor have constructors adding 'class/type' information in the future. E.g. the actual type-safe feature constructor or package can provide support for constructing details via parameters or alternative functions.
Pro and cons:
- Providing a constructor will always force anyone to update feature definitions once the constructor changes. Pro: always up to date meta-data, Cons: developer adding a field must update all features.
Cons:
- if we add more information to the details, it might become more difficult to see which fields are actually set. By using a struct it's always clear if one writes
Details{FullName: ..., Doc: ...} - By having
Describableand interface makes the code a little more bloated. Plus it enforces the use of the constructor, so to work around symbol-name reuse.
There was a problem hiding this comment.
I had the same indecision and a similar list.
My main reason I've opted for an interface is, is because data and need will surely change in a minor release and I wanted to make sure we reduce the changes needed by external developers.
libbeat/feature/bundle.go
Outdated
There was a problem hiding this comment.
How about updating the function to accept a predicate.
e.g.
func (b *Bundle) FilterWith(pred func(string, Describer) bool) *Bundle {
var filtered []Featurable
for _, feature := range b.features {
if pred(feature.Namespace(), feature.Description()) {
filtered = append(filtered, feature)
}
}
return NewBundle(filtered...)
}
Filtering for stability can be provided using a predicate constructor:
func HasStabilityPred(stabilities ...Stability) func(string, Describer) bool {
return func(ns string, d Describer) bool {
s := d.Stability()
for _, other := range stabilites {
if s == other {
return true
}
}
}
}
Then Filter becomes:
func (b *Bundle) Filter(stabilities Stability) *Bundle {
return b.FilterWith(HasStabilityPred(stabilities))
}
Having a FilterWith, we can collect Features within a given sub-namespace for example. Yet not sure if we will need it. Use cases coming to mind: for printing available features in a given namespace or passing a sub-set of a bundle to the module loaders (ensure a module does not access the wrong namespace).
There was a problem hiding this comment.
Im OK with this change, Not sure why this pattern never come to my mind.
There was a problem hiding this comment.
I like the idea, the following chain will be possible.
bundle.FilterWith(NamespaceMatchPred("processor"))
.FilterWith(HasStabilityPred(feature.Beta))|
@urso I've updated the PR with the initial review. Concerning the concrete type vs the interface for details is debatable. |
Motivation: Only adding a stability to describe a feature is often not enough, instead we now allow to add the following information. Fullname: This is a more human readable version of the feature. Example: Jolokia autodiscovery Doc: This is a one liner describing what you can do with the feature. Example: The dissect processor allows to extract useful parts of the original string. Stability: Describe how stable is the current feature.
Currently we only provide the HasStabilityPred to filter feature from a
bundle, but this change open the door to chainable filterWith.
Example:
```
bundle.FilterWith(NamespaceMatchPred("processor"))
.FilterWith(HasStabilityPred(feature.Beta))
```
|
Failure is unrelated to this PR changes, Related issue #7683 |
…re (#8571) Cherry-pick of PR #7703 to 6.x branch. Original message: **Motivation:** Only adding a stability to describe a feature is often not enough, instead we now allow to add the following information. **Fullname:** This is a more human readable version of the feature. Example: Jolokia autodiscovery **Doc:** This is a one liner describing what you can do with the feature. Example: The dissect processor allows to extract useful parts of the original string. **Stability:** Describe how stable is the current feature. From the conversation in #7443 (comment) to make the feature concept a bit more future proof. **Note:** I've decided to use an interface in that case because it will be easier to give an upgrade path for feature developers.
Motivation:
Only adding a stability to describe a feature is often not enough,
instead we now allow to add the following information.
Fullname:
This is a more human readable version of the feature.
Example: Jolokia autodiscovery
Doc:
This is a one liner describing what you can do with the feature.
Example: The dissect processor allows to extract useful parts of the
original string.
Stability:
Describe how stable is the current feature.
From the conversation in #7443 (comment) to make the feature concept a bit more future proof.
Note: I've decided to use an interface in that case because it will be easier to give an upgrade path for feature developers.