[Graph] Register graph usage #72041

flash1293 · 2020-07-16T12:04:04Z

This PR notifies the licensing plugin about usage of Graph apis.

@kobelb can you check whether it's used correctly?

kibanamachine · 2020-07-16T13:46:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 7212e10

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kobelb

Thanks for addressing this so quickly @flash1293. The integration looks good to me. I'm not too familiar with the Graph code to comment on whether or not you're calling licenseState.notifyUsage('Graph') in all situations, but the ones you have added LGTM.

kertal

Code LGTM, tested locally in Chrome, MacOS 10.14.6. notify function is triggered on API calls. Mission accomplished 👍

kertal · 2020-07-17T07:13:27Z

x-pack/plugins/graph/server/lib/license_state.ts

 import { ILicense } from '../../../licensing/common/types';
 import { checkLicense, GraphLicenseInformation } from '../../common/check_license';

 export class LicenseState {
  private licenseInformation: GraphLicenseInformation = checkLicense(undefined);
  private subscription: Subscription | null = null;
  private observable: Observable<GraphLicenseInformation> | null = null;
+  private _notifyUsage: LicensingPluginStart['featureUsage']['notifyUsage'] | null = null;


just an thought when viewing this, why not call it licenseNotifyUsage to omit the _

flash1293 · 2020-07-17T07:32:36Z

@kobelb based on mailing-list discussion, there are two things I'm unsure about:

On Elasticsearch side there is the explore API powering the Graph app (the Kibana APIs I tapped with notifyUsage in this PR are mostly just proxies for the explore API). However it is a completely valid use case for customers to consume the explore API directly via script or similar. Should we even implement Kibana side notifyUsage or is it fine to rely on ES to handle this?
If the question to the previous question is "we should capture it in Kibana", then I got a second question: As this is just happening on specific Graph APIs, the Graph app can theoretically be opened and existing workspaces can be viewed without notifying usage. Even deleting and rearranging existing nodes and saving the result would work this way. As soon as you start extending the graph (the actual core functionality of the application), the APIs will be hit. Is this still in the spirit of notifyUsage? We could work around this by bringing notifyUsage to the client by a Kibana API for this very purpose (api/graph/notifyUsage, called when the app is loaded), but it seemed like overkill to me.

kobelb · 2020-07-17T14:20:54Z

Should we even implement Kibana side notifyUsage or is it fine to rely on ES to handle this?

As long as we can always rely on Elasticsearch, we don't need to implement the notifyUsage in Kibana. There is some potential benefit that we get to doing so, though. If the user thinks of Graph as a "Kibana feature", reporting it along with Kibana's licensed feature usage potentially makes it more obvious to the end-user what feature they actually used. If this was only reported as part of Elasticsearch's features, it'd potentially confuse some users who only consume Graph via Kibana.

I think we should be using the guiding question of "Is this a Kibana feature?" to make this decision. In some situations, the answer is clearly yes, for example APM Service Maps which is only available via Kibana. In situations where Kibana simply provides management screens for Elastisearch features, for example role's with document-level-security and field-level-security, the answer is no. And in other situations, there isn't a clear answer, like Graph... It's both a Kibana feature and an Elasticsearch feature, so I think it's best to represent it that way in the licensed feature usage APIs.

As this is just happening on specific Graph APIs, the Graph app can theoretically be opened and existing workspaces can be viewed without notifying usage. Even deleting and rearranging existing nodes and saving the result would work this way. As soon as you start extending the graph (the actual core functionality of the application), the APIs will be hit. Is this still in the spirit of notifyUsage?

I think the way you've implemented this is fine. We'd like to know when a user substantially consumes one of our licensed features. If the user accidentally opens up the Graph application once to figure out what it was, I don't really consider this to be "actual usage". The fact that we're notifying usage based on the actual core functionality of the application is perfect. I could see us wanting usage to be notified when a user is rearranging existing nodes, but I don't think we absolutely have to. If my understanding is correct, if the user is actually using Graph, we'll be notified of their usage, which is the intent of this API.

We could work around this by bringing notifyUsage to the client by a Kibana API for this very purpose (api/graph/notifyUsage, called when the app is loaded), but it seemed like overkill to me.

When initially creating the licensed feature usage service, we decided to only implement it on the server. This was primarily out of laziness as we were unaware of any licensed Kibana features which we couldn't use a server-side method of determining when a feature is being actually used. I defended this decision with the reasoning that if we are only enforcing the licensing constraints in the browser, this is a rather weak enforcement that can generally be evaded by users, so we shouldn't be doing this in the first place. There's potential that there are errors in my thinking, and we should be exposing a client-side API, which would ideally be exposed by the licensing plugin. Do you feel that we're missing out on "substantial usage" by not being able to notify usage via the client-side? If so, I don't think we should block merging this PR, but we should be addressing the limitation in a subsequent PR.

flash1293 · 2020-07-17T15:16:20Z

Thanks for the clarifications, @kobelb. I do not think we are missing out on "substantial usage" this way. Going to merge as is.

* master: [Maps] 7.9 documenation updates (elastic#71893) docs: ✏️ add "Explore underlying data" user docs (elastic#70807) [Security Solution][Exceptions] - Remove initial add exception item button in builder (elastic#72215) Fix indentation level in code exploration doc (elastic#72274) register graph usage (elastic#72041) [Monitoring] Added a case for Alerting if security/ssl is disabled (elastic#71846)

* master: [Observability] Remove app logos (elastic#72259) Fix float percentiles line chart (elastic#71902) update chromedriver to 84 (elastic#72228) [esArchiver] actually re-delete the .kibana index if we lose recreate race (elastic#72354) [Maps] convert SavedGisMap to TS (elastic#72286) [DOCS] Removes occurrences of X-Pack Security and Reporting (elastic#72302) use WORKSPACE env var for stack_functional_integration tests, fix navigate path (elastic#71908) [Monitoring] Fix issue with ES node detail status (elastic#72298) [SIEM] Updates consumer in export_rule archive (elastic#72324) [kbn/dev-utils] add RunWithCommands utility (elastic#72311) [Security Solution][Endpoint][Exceptions] Only write manifest to policy when there are changes (elastic#72000) skip flaky suite (elastic#72339) [ML] Fix annotations pagination & change labels from letters to numbers (elastic#72204) [Lens] Fix switching with layers (elastic#71982) [Maps] 7.9 documenation updates (elastic#71893) docs: ✏️ add "Explore underlying data" user docs (elastic#70807) [Security Solution][Exceptions] - Remove initial add exception item button in builder (elastic#72215) Fix indentation level in code exploration doc (elastic#72274) register graph usage (elastic#72041) [Monitoring] Added a case for Alerting if security/ssl is disabled (elastic#71846)

register graph usage

7212e10

flash1293 added Feature:Graph Graph application feature v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Jul 16, 2020

flash1293 marked this pull request as ready for review July 16, 2020 15:08

flash1293 requested a review from a team July 16, 2020 15:08

flash1293 added the Team:Visualizations Visualization editors, elastic-charts and infrastructure label Jul 16, 2020

flash1293 requested a review from kobelb July 16, 2020 15:08

kobelb approved these changes Jul 16, 2020

View reviewed changes

kertal approved these changes Jul 17, 2020

View reviewed changes

flash1293 merged commit 825c168 into elastic:master Jul 17, 2020

flash1293 deleted the graph/feature-usage branch July 17, 2020 15:16

flash1293 added a commit to flash1293/kibana that referenced this pull request Jul 17, 2020

register graph usage (elastic#72041)

47a77b4

flash1293 mentioned this pull request Jul 17, 2020

[7.x] register graph usage (#72041) #72300

Merged

flash1293 added a commit that referenced this pull request Jul 20, 2020

register graph usage (#72041) (#72300)

b72320c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Graph] Register graph usage #72041

[Graph] Register graph usage #72041

flash1293 commented Jul 16, 2020

kibanamachine commented Jul 16, 2020

kobelb left a comment

kertal left a comment

kertal Jul 17, 2020

flash1293 commented Jul 17, 2020 •

edited

Loading

kobelb commented Jul 17, 2020

flash1293 commented Jul 17, 2020

[Graph] Register graph usage #72041

[Graph] Register graph usage #72041

Conversation

flash1293 commented Jul 16, 2020

kibanamachine commented Jul 16, 2020

💚 Build Succeeded

Build metrics

kobelb left a comment

Choose a reason for hiding this comment

kertal left a comment

Choose a reason for hiding this comment

kertal Jul 17, 2020

Choose a reason for hiding this comment

flash1293 commented Jul 17, 2020 • edited Loading

kobelb commented Jul 17, 2020

flash1293 commented Jul 17, 2020

flash1293 commented Jul 17, 2020 •

edited

Loading