-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
add licensed feature usage API #63549
add licensed feature usage API #63549
Conversation
Pinging @elastic/kibana-platform (Team:Platform) |
type CoreSetupMockType = MockedKeys<CoreSetup> & jest.Mocked<Pick<CoreSetup, 'getStartServices'>>; | ||
type CoreSetupMockType = MockedKeys<CoreSetup> & { | ||
getStartServices: jest.MockedFunction<StartServicesAccessor<any, any>>; | ||
}; |
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 encountered (yet another) issue with the getStartServices
mock type in tests. the mock StartServicesAccessor
is now explicitly typed as any, any
.
(res, [featureName, lastUsage]) => { | ||
return { | ||
...res, | ||
[featureName]: new Date(lastUsage).toISOString(), |
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.
toISOString
returns the expected ISO 8601
format, so I did not see any reason to use moment
instead.
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.
This may have already been discussed but I wonder if there could be any problems with using absolute dates. If clocks are not configured correctly, it could make making sense of this data harder from the Cloud side. One option could be to return a relative date, ie. how many seconds ago it was used.
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 comment as #63549 (comment), not sure if we can change the expected format here.
One option could be to return a relative date, ie. how many seconds ago it was used
This has different drawbacks btw, such as being vulnerable to latency, and forcing to be processed on the fly or enhanced to not loose the time reference of the consumer call.
Also I would be expecting cloud instances to have properly synchronized clocks between their machines/vms/cris so this is probably a non-issue?
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.
Another problem could be that a user couldn't know in what timezone the timestamp was created. Not sure if it's a critical problem.
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 assumed it was not an issue, as the API consumption is just going to pool calls to the API to count the actual number of usages in a given period of time. TBH I would have returned a unix timestamp instead of an ISO date representation if the elasticsearch API counterpart was not returning this format.
If we want to have correct timezones, I can just change the feature usage service to store dates instead of unix timestamps, but once again, this is assuming the actual user/client and the kibana server are on the same timezone.
My main difficulty on this feature is that it's very unclear who has the power of decision on that API.
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.
Also I would be expecting cloud instances to have properly synchronized clocks between their machines/vms/cris so this is probably a non-issue?
I think this is a safe enough assumption for its usage. There's always the potential for clock drift, but even if we're off a bit it won't have catastrophic effects.
Using an ISO-8601 format for dates is 👍 , they can be easily translated to a unix timestamp. Date::toISOString
doesn't include the timezone offset, which is completely fine in this situation as the timezone that the server is running in is irrelevant.
clear: () => { | ||
this.lastUsages.clear(); | ||
}, |
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'm not really happy to have to expose this as a public API, however I needed a way to reset the state of the service for FTR tests from a plugin.
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.
Probably we can do it later when (if) necessary? As I can see, you call it only once in the functional tests.
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.
My reasoning behind adding this was that is this is merged without the clear, this will fails on any PR implementing usages of this API, as the
expect(response.body).to.eql({
test_feature_a: toISO(timeA),
test_feature_b: toISO(timeB),
});
assertion would fails if another entry is present.
But I guess I can change this assertion to something checking the individual keys presence / value instead of asserting the whole structure.
Will remove clear
.
CI failures are caused by #61652 and should not block reviews. |
(res, [featureName, lastUsage]) => { | ||
return { | ||
...res, | ||
[featureName]: new Date(lastUsage).toISOString(), |
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.
This may have already been discussed but I wonder if there could be any problems with using absolute dates. If clocks are not configured correctly, it could make making sense of this data harder from the Cloud side. One option could be to return a relative date, ie. how many seconds ago it was used.
const currentValue = this.lastUsages.get(featureName) ?? 0; | ||
this.lastUsages.set(featureName, Math.max(usedAt, currentValue)); | ||
}, | ||
getLastUsages: () => new Map(this.lastUsages.entries()), |
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.
Should this return null
values for features that were registered but have not been used? As it is right now, features that have not been used do not show up in the API response at all. I'm not sure what consumers of this expect, but it may be a bit clearer if all the tracked features show in this list, regardless of usage.
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'm following @rjernst 'specifications' from #60984 (comment) here. non-used features does not appear in this API apparently.
I'm unsure what liberty we have on that, as the goal seems to be mirroring ES' WIP api?
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 we need features
list in this case at all? Can we removeregister
call?
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 we need features list in this case at all? Can we remove register call?
That was my first thought as well. If we don't need to return the list of all features (even if unused) then we could probably remove it. Though it does make it a bit safer that plugins can't just shove random strings into the notifyUsage
API.
Maybe we could get away with a union string type for the allowed strings, though that would mean plugins have to update these types in order to add new usage tracking.
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 we need features list in this case at all? Can we remove register call?
There is no functional reasons for this call atm. I just added it because it felt more KP compliant for plugins to explicitly register every licensed feature they wanted to notify usages of. If we think this is useless, I can definitely remove that part.
Maybe we could get away with a union string type for the allowed strings
Mixed on that. In addition to that would mean plugins have to update these types in order to add new usage tracking
, this also kinda breaks the feature for 3rd party plugins, as they wouldn't be able to register new features, even if realistically it's probably a non-issue.
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.
Though it does make it a bit safer that plugins can't just shove random strings into the notifyUsage API.
What prevents them from passing a random string to register
as well?
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.
Nothing, but at least they can only notify usage of the arbitrary features that were registered.
import { FtrProviderContext } from '../../ftr_provider_context'; | ||
|
||
export default function({ loadTestFile }: FtrProviderContext) { | ||
describe('Resolver embeddable test app', function() { |
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.
Might want to rename 😄
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.
🤦
export interface FeatureUsageServiceStart { | ||
/** | ||
* Notify of a registered feature usage at given time. | ||
* If `usedAt` is not specified, it will use the current time instead. |
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.
Should we add a comment that usedAt
is expected to be in UNIX timestamp format? Or even accept the Date
object.
const currentValue = this.lastUsages.get(featureName) ?? 0; | ||
this.lastUsages.set(featureName, Math.max(usedAt, currentValue)); | ||
}, | ||
getLastUsages: () => new Map(this.lastUsages.entries()), |
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 we need features
list in this case at all? Can we removeregister
call?
|
||
it('returns a map of last feature usages', async () => { | ||
const timeA = Date.now(); | ||
await notifyUsage('test_feature_a', timeA); |
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.
Do we need await
on the function call?
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 we do, else we wont necessarily have the call finished before querying /api/licensing/feature_usage
down in the test?
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.
my bad, I missed it's a remote call
(res, [featureName, lastUsage]) => { | ||
return { | ||
...res, | ||
[featureName]: new Date(lastUsage).toISOString(), |
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.
Another problem could be that a user couldn't know in what timezone the timestamp was created. Not sure if it's a critical problem.
clear: () => { | ||
this.lastUsages.clear(); | ||
}, |
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.
Probably we can do it later when (if) necessary? As I can see, you call it only once in the functional tests.
"id": "feature_usage_test", | ||
"version": "1.0.0", | ||
"kibanaVersion": "kibana", | ||
"configPath": ["xpack", "feature_usage_test"], |
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.
Name collision is unlikely, but I'd rather avoid it.
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.
Not fully convinced register
method is useful, but I'm okay if you consider it necessary.
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.
The current implementation doesn't make any association between a feature's usage and its minimum license level. This is potentially something we can push to the consumer of the API, primarily Cloud in this situation, but they'll then be forced to know which features are available at which license level.
@suyograo @rjernst should the feature usage API in Kibana be returning the minimum license for each feature, or is this a responsibility Cloud will take on?
My understanding from our previous conversations is this would be the responsibility of Cloud to map. |
@rjernst are the equivalent Elasticsearch APIs going to be returning features for all license levels, including the Basic license? Initially, I was thinking that we'd only need to change the consumers of Gold+ features to track their usage, but that seems wrong. |
Yes, currently it is my plan to provide this for all features, regardless of license level. |
I could change the
Even if this would not be used atm, this would futur-proof us if the need arises to add this info in the public API or to filter features returned from the API? Else I'm probably going to follow @restrry suggestion and just remove this |
With the way that Kibana's licensing plugin is currently structured, it seems rather difficult for us to have parity with the Elasticsearch APIs. The licensing plugin exposes an Additionally, the way you've structured the feature usage service makes sense given the current implementation of licensing. However, I'm afraid that it will lead to developers not using it properly and bugs. If we want every feature's usage to be returned by the If instead of exposing an
|
Yea. From your first answer in #60984 (comment), I thought that was the expected architecture, but I don't mind reopening the discussion. I agree that this could lead to developer errors. OTOH I feel like this feature is something targeting only a few specific features, which make me think that maybe impacting / changing the way more commonly used license checking feature for this is an unnecessary risk?
Putting aside the fact that this is an heavy impacting change/proposal for a feature that is supposed to be shipped in 7.8: I actually like the idea, however I'm mixed on your implementation. Changing the current async/observable approach with a synchronous 'feature' wrapper is elegant, however I feel like some edge cases are not going to be properly covered: switching from async to sync accessthe license is not instantly accessible as it is asynchronously fetched with an interval from ES. const license = await license$.pipe(take(1)).toPromise();
if(!checkFeature(license)) {
// do something
} is assured to be resolved only when the initial license fetch/check has been performed, where const licensedFeatures = licensing.createLicensedFeatures('security', {
feature: 'basic',
});
const shouldHideSomething = licensedFeatures.check('feature', false); is not. Meaning that I don't think we can realistically have a synchronous API for feature checking. Probably just having this API asynchronous instead solves this. const licensedFeatures = licensing.createLicensedFeatures('security', {
feature: 'basic',
});
const shouldHideSomething = await licensedFeatures.check('feature', false); loosing the reactivityFollow up of previous point. The license state/type can change during the lifecycle of the application. The license is an observable for this very reason, as It allow plugins to react to these changes of state. Meaning that going back to a simple async We could go with something like const licensedFeatures = licensing.createLicensedFeatures('security', {
feature: 'basic',
});
const unsub = licensedFeatures.onFeatureStateChange('feature', ({ status, notifyUsage }) => {
if(status === 'enabled') {
notifyUsage()
} else {
hideSomething();
}
}); While keeping the public / server API consistencyATM, the licensing plugin API is kinda consistent between client and server, in the sense that both sides exposes an Option 1 means adding additional client/server communication/endpoints to be able to
about the features naminglicensing.createLicensedFeatures('security', {
rbac: 'basic',
subFeaturePrivileges: 'gold',
auditLogging: 'gold'
}); This this call, what are the expected feature name when calling the usage API? Regarding actual usages of the licensing plugin APII don't have enough knowledge of the Adapted initial proposaltype Unregister = () => void;
type LicensedFeatureChecker<FeatureNames extends string> = {
check: (featureName: FeatureNames, notifyUsage: boolean) => Promise<boolean>;
observe: (
featureName: FeatureNames,
handler: (status: string, notifyUsage: () => void) => void
) => Unregister;
observeAlternative(
featureName: FeatureNames
): Observable<{ status: string; notifyUsage: () => void }>;
};
type LicensingPluginSetup = {
createLicensedFeatures: <T extends Record<string, LicenseType>>(
prefix: string, // should we just use the pluginId for that, or is it even necessary?
features: T
) => LicensedFeatureChecker<Extract<keyof T, string>>;
}; @restrry you probably have a way better vision than me on this. wdyt about @kobelb proposal? |
Some additions to @pgayvallet points:
const licensedFeatures = licensing.createLicensedFeatures('security', {
rbac: 'basic',
subFeaturePrivileges: 'gold',
auditLogging: 'gold'
}); since it doesn't cover all the cases for the licensing statuses:
It rather enriches the licensing data.
So my question is: Do we want to gather Kibana or Elasticsearch feature usages? // exists **only** within a plugin scope
const pluginFeatues = licensing.createPluginFeatures({
sub-feature: (license: ILicense) => ....,
});
// in an endpoint
if(
context.plugin.license.isAvailable &&
// automatically reports sub-feature usage if returns true
pluginFeatues.check('sub-feature')
){
return res.ok(...)
}
return res.forbidden(...) Or we could formalize Kibana Feature concept to highlight the difference between the terms: class KibanaFeature {
constructor(license$: Observable<ILicense>){
license$.subscribe(license => {
this.valid = license.isAvailable && license.hasAtLeast('gold');
})
}
eligibleForUsage(){
return this.valid
}
notifyUsage() {}
}
class RbacFeature extends KibanaFeature {
doSomething(){
this.notifyUsage()
return ...
}
} This is quite different from Licensing API usage when a license level check is done once on every license update. I'm not sure we should refactor all those places. |
During our initial conversation, it seemed reasonable to have the usage be a completely separate method call. After seeing the proposed implementation, I'm concerned with the repercussions though... I'm afraid that developers will forget to make the second method call it and it makes it difficult for us to have feature parity with the Elasticsearch API which reports all features. I'm assuming having this API return all features isn't necessary for the Cloud integration, as they are only concerned with a subset of these features. It's more-so the future maintainability of the solution that I'm worried about. I apologize for the earlier oversights and not realizing these concerns earlier. With regard to the timing, we have a few options... We can continue to use a stop-gap solution which relies on the telemetry endpoint in Kibana to derive the feature usage, or we can use this proposed solution for the short-term. I just think that long-term, we should be implementing a more fully fleshed out solution. Both of you brought up some great points and deficiencies in my prior thinking, particularly the loss of reactivity to the license changing, and losing the differentiation between Kibana and Elasticsearch features. ReactivityWith regard to reactivity into the license changing, we don't necessarily have to get rid of the use of Observables, like my initial proposal did. We could always expose the ability to create an It's my understanding that we currently recommend using the context provider APIs to make these Observables easier to consume, and I don't necessarily want to question that design decision at this point. Kibana vs Elasticsearch featuresWe're primarily concerned with Kibana specific feature usage in this situation. There are still situations where plugins will need to see which Elasticsearch features are available and enabled, so I agree that not exposing the If we keep the scope of this PR to addressing "licensed Kibana features" and their usage, and we alright with leaving the |
I think many of the concerns raised here by @kobelb are things we've talked about more broadly. In essence, integrating with licensing correctly is still hard, and we need to make it less so. I think improving the entire licensing integration experience needs to be thought from the ground-up. When licensing was migrated out of legacy, many improvements were made, but I think there is still more opportunity to provide utilities over the low-level APIs to make the common use cases easy to implement (eg. #56926). However, I also don't think we have time to fix this in the short-term for 7.8, especially considering that we still need plugins to integrate with this in the next 2 weeks. Let's go ahead with the current API design, but let's also make sure the HTTP interface is future-proof and won't have to significantly change in the future (I don't see any current problems with that design, but let's double check that). @pgayvallet Would you mind summarizing the issues raised in this thread into a high-level "improve licensing" issue? |
👍 |
Will do before merging this initial implementation, which should be done ASAP |
Created #64619 |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* add licensed feature usage API * add FTR test and plugin * jsdoc * fix FTR test suite name * remove clear API * accept Date for notifyUsage
@kobelb this just got merged. should you/we actively ping teams that need to implement this for 7.8? |
Summary
Fix #60984
Add a
featureUsage
service to the xpacklicensing
plugin and contracts, that can be used to register licensed features and notify of their usages, and a new/api/licensing/feature_usage
endpoint to collect last usage time of the registered features.Checklist