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
feat: Add feature toggles (DSP-910) #1742
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for implementing this. I just see the following problem about not being able to specify the version number when turning a toggle off:
Imagine we have initial newListRoute:1
and months later we add newListRoute:2
both are by default off. After a while, we decide to make a release with newListRoute:1
by default on, but newListRoute:2
to be by default off because other subsystems still need time to catch up with changes of newListRoute:2
.
Now, imagine a client pulls this release and is ready with support of newListRoute:2
and wants to use it. He has to turn newListRoute:1
off and turn newListRoute:2
on. If he cannot specify the version of the toggle that must be off, as it is now to achieve what he wants he needs to do
X-Knora-Feature-Toggles: new-foo=off, new-foo:2=on
that is kind of confusing. I would suggest specification of version for turning a toggle off to be possible, i.e. user must be able to set
X-Knora-Feature-Toggles: new-foo:1=off, new-foo:2=on
License along with Knora. If not, see <http://www.gnu.org/licenses/>. | ||
--> | ||
|
||
# Feature Toggles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very well explained!
docs/03-apis/feature-toggles.md
Outdated
``` | ||
|
||
A version number must be given when enabling a toggle. | ||
It is an error to specify a version number when disabling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well, in my opinion, the user should be able to specify which version of the toggle must be disabled.
``` | ||
|
||
A version number must be given when enabling a toggle. | ||
It is an error to specify a version number when disabling |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above!
/** | ||
* A user representing the Knora API server, used for initialisation on startup. | ||
*/ | ||
private val systemUser = KnoraSystemInstances.Users.SystemUser |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was unused.
/** | ||
* Indicates that a feature toggle is off. | ||
*/ | ||
case object Off extends FeatureToggleState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the reason explained above, I believe Off
state must also have the version number as Off(version:Int)
to indicate which version of a feature toggle is off.
/** | ||
* A matchable object indicating that a feature toggle is off. | ||
*/ | ||
case object Off extends MatchableState[Nothing] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
following the discussion above, I believe Off state should also know the version number
overrideAllowed: Boolean, | ||
expirationDate: Option[Instant], | ||
developerEmails: Set[String]) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add empty line here
* @param overrideAllowed `true` if this configuration can be overridden, e.g. by per-request feature | ||
* toggle configuration. | ||
* @param expirationDate the expiration date of the feature. | ||
* @param developerEmails one or more email addresses of developers who can be contacted about the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure why we need the developer's email? since feature toggles last a year, maybe until it is removed a developer quits, then his email address would be a useless piece of information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's useful to know who to contact if you have questions about a feature. If a developer quits, we can update the email address.
} | ||
} | ||
|
||
"not accept a version number when disabling a toggle" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not?
This would be done with a base configuration like this:
No, he just has to request:
I think it wouldn't make sense to specify a version when disabling a feature, because you can't enable more than one version at a time. It's not possible to enable both
To turn on one version of the feature, you just enable it, specifying the version you want. There's no need to disable the other versions, because you can only get one version. |
Your example is exactly what's happening in this test: |
@SepidehAlassi I've added docs to clarify things, and added a test. Is this better? |
yes, it is very clear now, thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, it looks good to me.
docs/03-apis/feature-toggles.md
Outdated
|
||
# Feature Toggles | ||
|
||
Some Knora features can be turned or off on a per-request basis. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess: "Some Knora features can be turned on or off on a per-request basis."?
came from. | ||
|
||
- An HTTP response should indicate which features are turned | ||
on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the response header also going to return turned off toggles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently it only returns enabled toggles. I was thinking that if a toggle is turned off by default because the feature isn't ready to use yet, there wouldn't be much point in returning it. But maybe it would be clearer and simpler to return everything. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think returning everything might not be a good idea. Since the lifetime of toggles is a year, we might have many (>10 even) toggles in place that are turned off but still available. IMO, returning all of them would confuse the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the only time this header might get looked at by a developer is if there is a problem. In that case, I would probably like to have the whole picture. Ideally, there would also be something like x-knora-api-version
. Those two headers would then paint the whole picture (in combination with a table in the documentation).
As for the confusion bit, well toggles are going to add another layer of complexity for understanding what is happening with the API. Hiding them, would in this case probably provide more confusion, because a turned off toggle, which should maybe be on can be valuable information during debugging.
should have expiration dates except for long-lived ops toggles like `fast-baz` above. | ||
|
||
`KnoraSettingsFeatureFactoryConfig` reads this base configuration on startup. If | ||
a feature toggle has an expiration date in the past, the application will not start. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, this is a hard one. I wouldn't punish ops for developers not removing a toggle in time ;-) Also what should then happen, if the expiration date is passed while the app is deployed into production. I hope the app will not stop working? I think a log output would be sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to:
the application will log the error message "SHOW-STOPPER!!!" and the server will then self-destruct
Just kidding. 😃
/** | ||
* The old lists route feature. | ||
*/ | ||
private val oldListsRouteADMFeature = new OldListsRouteADMFeature(routeData) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we maybe move the factory and features to a sub-package, e.g., org.knora.webapi.routing.admin.lists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this :-)
@benjamingeer I keep thinking about how we are going to add non-breaking new features, can you please not merge this PR until we discuss this thoroughly. Please imagine the following scenario:
I would appreciate knowing your opinion on this, it can well be that I am missing a fourth perfect option. |
Does it probably depend on the granularity of a feature toggle? In the case of Or, the implementation is broken down one more level, where each subroute goes into its separate file and the factory part is only referencing the separate implementations. Those separate implementations are then unit tested. I'm sure, that there are a lot more questions open, that we didn't think of yet. We need to jump into the cold water and see what happens. |
- Move lists features into subpackage. - If a feature toggle is expired, just log a warning. - Update docs.
I've made the format of the request and response headers identical (listing all toggles in the response header), because I think this will make the response header easier to read: you don't have to figure out that something is turned off because it's not there, you can see that it's turned off. I changed I moved the list factory into a sub-package. @subotic you wrote:
Doesn't the existing server version header include that? @SepidehAlassi About non-breaking new features, I agree with Ivan. In general I think it wouldn't be bad to go with your option (2), especially if we do as Ivan suggests:
That would reduce code duplication. But maybe we can take a different approach in each case, depending on how urgent the new feature is, how much work is involved in implementing support for the breaking changes, etc. If it's OK with both of you, I'll merge this now. |
Fine with me :-) Thanks! |
@benjamingeer it is also fine with me, thanks for all your work. |
Many thanks to both of you for the very helpful reviews! |
FeatureFactory.scala
,KnoraSettings.scala
, andKnoraRoute.scala
ListsRouteADM
using a feature toggle (to be used for DSP-597)v2
andadmin
routes so they can use feature togglesR2RSpec
so tests can override the application configurationFeatureToggleR2RSpec
Resolves DSP-910.