Skip to content
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

[Telemetry] Have Telemetry automatically get all the Kibana usage stats #22336

Merged
merged 13 commits into from
Oct 22, 2018

Conversation

tsullivan
Copy link
Member

Closes #21239

This change gets the telemetry data payload to automatically include any usage stat data that has been collected in the .monitoring-kibana-* indices.

Note that Kibana telemetry still only works when Kibana Monitoring is turned on.

@tsullivan
Copy link
Member Author

@pickypg this PR has the stats generated by Kibana's usage collectors generically added into a kibana.plugins object. For example:

  "kibana": {
    "count": 1,
    "dashboard": { "total": 1 },
    "visualization": { "total": 1 },
    "search": { "total": 0 },
    "index_pattern": { "total": 3 },
    ...,
    "plugins": {
     "canvas": {
      "workpads": { "total": 2 },
      ...
     },
     "xpack": {
      "reporting": {
       "available": true,
       "enabled": true,
       "browser_type": "phantom",
       ...
      }
     }
    }
   }

Previously we had a module to collect reporting stats, and there is an overall combiner that fetches reporting stats and plugs it into the payload in an area we decided on. This setup isn't scalable as we need to add another module for each new integration.

The obvious issue is this breaks the expected data that the telemetry web service receives. Can something be changed there so we can go with these changes?

@pickypg
Copy link
Member

pickypg commented Aug 24, 2018

The obvious issue is this breaks the expected data that the telemetry web service receives. Can something be changed there so we can go with these changes?

We can adjust the other side to handle the breaking changes if we must. Let's not get too harsh of an issue, but we should avoid continuing down this path for future telemetry and move to the new model in the future (for others reading this).

@tsullivan
Copy link
Member Author

retest

@elastic elastic deleted a comment from elasticmachine Aug 28, 2018
@elastic elastic deleted a comment from elasticmachine Aug 28, 2018
@tsullivan tsullivan changed the title [Telemetry/Wip] Have Telemetry automatically get all the Kibana usage stats [Telemetry] Have Telemetry automatically get all the Kibana usage stats Aug 28, 2018
@elastic elastic deleted a comment from elasticmachine Aug 28, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan
Copy link
Member Author

@pickypg able to take a look for review?

@tsullivan
Copy link
Member Author

cc @elastic/kibana-platform anyone have some time to review this change to simplify the payload-generation side of telemetry?

@tsullivan
Copy link
Member Author

tsullivan commented Sep 4, 2018

We can adjust the other side to handle the breaking changes if we must. Let's not get too harsh of an issue, but we should avoid continuing down this path for future telemetry and move to the new model in the future (for others reading this).

Maybe we can keep this conversation going. I get that it is a pain to add more mappings in the telemetry cluster for new features added into the data. But that's only half the pain, this PR addresses the caller's half of the pain of wiring new features into the data.

I think I understand the new model better than I initially did. Am I right in thinking that the data could be fit into that model in a post-query phase? I'm pretty ok with how the usage collectors represent the data they fetch, but perhaps that data could be mapped right before getting transmitted to have an expressive structure where mapping on the other side would be automatic.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan
Copy link
Member Author

We can adjust the other side to handle the breaking changes if we must. Let's not get too harsh of an issue, but we should avoid continuing down this path for future telemetry and move to the new model in the future (for others reading this).

I'm coming back to this as other work targeted for 6.5 depends on this. The update on this comment is that this PR is planned to be the last breaking thing we'll do for the telemetry upload data.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@tsullivan
Copy link
Member Author

re: #22336 (comment)

    "xpack": {
      "reporting": {

Correction/update: there will be no plugins nested under xpack. That was a side-effect of having the internal monitoring collection keep the structure that was in place for reporting. Since this breaks the data model already (moving reporting into kibana.plugins from making the stats merged together in a generic way, let's make it really generic and not differentiate xpack.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@pickypg pickypg left a comment

Choose a reason for hiding this comment

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

LGTM with latest tweak

@tsullivan tsullivan merged commit c4cb820 into elastic:master Oct 22, 2018
@tsullivan tsullivan deleted the telemetry/get-kibana-stats-all branch October 22, 2018 18:28
tsullivan added a commit to tsullivan/kibana that referenced this pull request Oct 22, 2018
…ts (elastic#22336)

* Have Telemetry automatically get all the Kibana usage stats

* simplify

* remove reporting module

* remove unused helper function

* fix tests

* fix integration tests

* --wip-- [skip ci]

* getKibanaStats flattens nested xpack plugin stats into a single level

* fix integration test
tsullivan added a commit that referenced this pull request Oct 22, 2018
…ts (#22336) (#24353)

* Have Telemetry automatically get all the Kibana usage stats

* simplify

* remove reporting module

* remove unused helper function

* fix tests

* fix integration tests

* --wip-- [skip ci]

* getKibanaStats flattens nested xpack plugin stats into a single level

* fix integration test
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.

3 participants