Skip to content

Adding more flexibility to describe a feature#7703

Merged
urso merged 5 commits intoelastic:masterfrom
ph:feature/use-details-instead-of-only-stability
Jul 27, 2018
Merged

Adding more flexibility to describe a feature#7703
urso merged 5 commits intoelastic:masterfrom
ph:feature/use-details-instead-of-only-stability

Conversation

@ph
Copy link
Copy Markdown
Contributor

@ph ph commented Jul 24, 2018

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.

@ph ph added the libbeat label Jul 24, 2018
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ph ph added the review label Jul 24, 2018
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's common in go to end interface names with er. E.g. Describer

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we need String for? Is it supposed to be the short name?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed but details will still implement the stringer interface.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.Stable or 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 Describable and interface makes the code a little more bloated. Plus it enforces the use of the constructor, so to work around symbol-name reuse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@urso urso Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im OK with this change, Not sure why this pattern never come to my mind.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea, the following chain will be possible.

bundle.FilterWith(NamespaceMatchPred("processor"))
   .FilterWith(HasStabilityPred(feature.Beta))

@ph
Copy link
Copy Markdown
Contributor Author

ph commented Jul 26, 2018

@urso I've updated the PR with the initial review.

Concerning the concrete type vs the interface for details is debatable.
I still think we should keep the interface, this potentially gives us more flexibility especially in the transition of 6.x vs 7.0.

ph added 5 commits July 26, 2018 14:57
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))
 ```
@ph
Copy link
Copy Markdown
Contributor Author

ph commented Jul 27, 2018

Failure is unrelated to this PR changes, Related issue #7683

@urso urso merged commit 91af9a7 into elastic:master Jul 27, 2018
@ph ph added needs_backport PR is waiting to be backported to other branches. and removed review labels Oct 4, 2018
@ph ph added v6.5.0 and removed needs_backport PR is waiting to be backported to other branches. labels Oct 4, 2018
ph added a commit that referenced this pull request Oct 5, 2018
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants