[Ingest Manager] Add usage collector for telemetry.#69294
Merged
skh merged 18 commits intoelastic:masterfrom Jul 8, 2020
Merged
[Ingest Manager] Add usage collector for telemetry.#69294skh merged 18 commits intoelastic:masterfrom
skh merged 18 commits intoelastic:masterfrom
Conversation
7b51935 to
a378530
Compare
32f5b18 to
b3f011f
Compare
Contributor
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
Contributor
Author
Any advice from @elastic/telemetry here is more than welcome! |
jasonrhodes
reviewed
Jul 7, 2020
Member
There was a problem hiding this comment.
Hi! Butting in here just to say I'm curious about this ... is there a reason you need this "internal repository" vs just savedObjects.client ? And if you do, what's does createInternalRepository() return before you re-cast it as unknown?
Contributor
Author
There was a problem hiding this comment.
I was working from this example code:
// server/plugin.ts
import { UsageCollectionSetup } from 'src/plugins/usage_collection/server';
import { CoreSetup, CoreStart } from 'kibana/server';
class Plugin {
private savedObjectsRepository?: ISavedObjectsRepository;
public setup(core: CoreSetup, plugins: { usageCollection?: UsageCollectionSetup }) {
registerMyPluginUsageCollector(() => this.savedObjectsRepository, plugins.usageCollection);
}
public start(core: CoreStart) {
this.savedObjectsRepository = core.savedObjects.createInternalRepository();
}
}
(from https://github.com/elastic/kibana/blob/master/src/plugins/usage_collection/README.md#new-platform ) and poked at it until it started working.
createInternalRepository() returns a SavedObjectsRepository.
Trying to use it in place of a SavedObjectClient results in this type mismatch:
const soClient: Pick<SavedObjectsRepository, "create" | "bulkCreate" | "delete" | "deleteByNamespace" | "find" | "bulkGet" | "get" | "update" | "addToNamespaces" | "deleteFromNamespaces" | "bulkUpdate" | "incrementCounter">
Argument of type 'Pick<SavedObjectsRepository, "create" | "bulkCreate" | "delete" | "deleteByNamespace" | "find" | "bulkGet" | "get" | "update" | "addToNamespaces" | "deleteFromNamespaces" | "bulkUpdate" | "incrementCounter">' is not assignable to parameter of type 'SavedObjectsClient'.
Type 'Pick<SavedObjectsRepository, "create" | "bulkCreate" | "delete" | "deleteByNamespace" | "find" | "bulkGet" | "get" | "update" | "addToNamespaces" | "deleteFromNamespaces" | "bulkUpdate" | "incrementCounter">' is missing the following properties from type 'SavedObjectsClient': errors, _repository
Member
There was a problem hiding this comment.
Maybe the @elastic/kibana-platform team can help with this type confusion (personally I also find it hard to understand all the differences between SavedObjectsClient, ISavedObjectsRepository and SavedObjectsClientContract :) ).
Contributor
Author
There was a problem hiding this comment.
Summary of a Zoom discussion: in the usageCollector case we don't have a Kibana request, so we can't use CoreStart.savedObjects.getScopedClient() to get a client.
Member
There was a problem hiding this comment.
Could we try something like this:
import { SavedObjectsClient } from '../../../core/server'; // exact path TBD
export async function getInternalSavedObjectsClient(core: CoreSetup) {
return core.getStartServices().then(async ([coreStart]) => {
const savedObjectsRepo = coreStart.savedObjects.createInternalRepository();
return new SavedObjectsClient(savedObjectsRepo);
}
}
Member
There was a problem hiding this comment.
If something like this works, we preserve the "Saved Objects Client" contract needed for the injected methods, and we still create our own "internal" repo for saved objects which will give us access to all objects in all spaces with this client, rather than it being scoped. This relationship feels more intuitive than using the repo directly, also. (I'm not sure if we have a way to get a scoped version but we've suggested to the Telemetry team that it might be helpful to provide this to the fetch method, similar to the way the ES client is already provided there.)
Contributor
Author
There was a problem hiding this comment.
This works in local testing:
import { CoreSetup } from 'kibana/server';
import { SavedObjectsClient } from '../../../../../src/core/server';
export async function getInternalSavedObjectsClient(core: CoreSetup) {
return core.getStartServices().then(async ([coreStart]) => {
const savedObjectsRepo = coreStart.savedObjects.createInternalRepository();
return new SavedObjectsClient(savedObjectsRepo);
});
}
Waiting for feedback from @rudolf if it's safe to override the restricted paths error here. Thanks for the help @rudolf !
Member
There was a problem hiding this comment.
Looks great! I think that kibana/server is a magic shortcut to that src/core/server path so you might be able to just push the SavedObjectsClient up into that import with CoreSetup, or at least collapse both imports into whichever format you prefer and just have one import line (if I'm not mistaken on this).
Contributor
Author
There was a problem hiding this comment.
When importing SavedObjectsClient from kibana/server instead I get this error during yarn start (and yes, I did clean and bootstrap):
log [14:38:15.704] [fatal][root] { Error: Cannot find module 'kibana/server'
at Function.Module._resolveFilename (internal/modules/cjs/loader.js:636:15)
at Module.Hook._require.Module.require (/home/skh/projects/kibana/node_modules/require-in-the-middle/index.js:51:29)
at require (internal/modules/cjs/helpers.js:25:18)
at Object.<anonymous> (/home/skh/projects/kibana/x-pack/plugins/ingest_manager/server/collectors/helpers.ts:7:1)
at Module._compile (internal/modules/cjs/loader.js:778:30)
at Module._compile (/home/skh/projects/kibana/node_modules/pirates/lib/index.js:99:24)
at Module._extensions..js (internal/modules/cjs/loader.js:789:10)
at Object.newLoader [as .ts] (/home/skh/projects/kibana/node_modules/pirates/lib/index.js:104:7)
at Module.load (internal/modules/cjs/loader.js:653:32)
at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
at Function.Module._load (internal/modules/cjs/loader.js:585:3)
at Module.require (internal/modules/cjs/loader.js:692:17)
at Module.Hook._require.Module.require (/home/skh/projects/kibana/node_modules/require-in-the-middle/index.js:70:39)
at Module.Hook._require.Module.require (/home/skh/projects/kibana/node_modules/require-in-the-middle/index.js:70:39)
at Module.Hook._require.Module.require (/home/skh/projects/kibana/node_modules/require-in-the-middle/index.js:70:39)
at Module.Hook._require.Module.require (/home/skh/projects/kibana/node_modules/require-in-the-middle/index.js:70:39) code: 'MODULE_NOT_FOUND' }
FATAL Error: Cannot find module 'kibana/server'
I did not dig deeper, to be honest.
nchaulet
reviewed
Jul 7, 2020
Member
There was a problem hiding this comment.
Do you think you can use the method we have in appContextService like this https://github.com/elastic/kibana/blob/master/x-pack/plugins/ingest_manager/server/services/agents/checkin/state_new_actions.ts/#L33-L50 ?
Contributor
Author
There was a problem hiding this comment.
Do I already have access to the app context in setup()?
Contributor
|
Thanks @skh! |
afharo
approved these changes
Jul 7, 2020
Member
afharo
left a comment
There was a problem hiding this comment.
LGTM!
I'll try to fix the schema issues to support arrays!
x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts
Outdated
Show resolved
Hide resolved
Member
There was a problem hiding this comment.
Maybe the @elastic/kibana-platform team can help with this type confusion (personally I also find it hard to understand all the differences between SavedObjectsClient, ISavedObjectsRepository and SavedObjectsClientContract :) ).
Contributor
Author
|
@jen-huang @nchaulet While the discussion about how best to get a |
nchaulet
reviewed
Jul 7, 2020
x-pack/plugins/ingest_manager/server/collectors/agent_collectors.ts
Outdated
Show resolved
Hide resolved
nchaulet
reviewed
Jul 7, 2020
x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts
Outdated
Show resolved
Hide resolved
nchaulet
approved these changes
Jul 7, 2020
Member
nchaulet
left a comment
There was a problem hiding this comment.
A few comment on variables naming but looks good to me
2 tasks
afharo
reviewed
Jul 7, 2020
Comment on lines
44
to
57
Contributor
Author
There was a problem hiding this comment.
👍 thanks! Should I wait for that one to be merged and rebase, or is it ok if I add our schema in a followup PR?
…tors.ts Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com>
…tors.ts Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com>
c31f43d to
c226897
Compare
Closed
jasonrhodes
approved these changes
Jul 8, 2020
Member
jasonrhodes
left a comment
There was a problem hiding this comment.
LGTM, just had a comment about the import paths. Hooray for telemetry!
Contributor
💛 Build succeeded, but was flaky
Test FailuresFirefox UI Functional Tests.test/functional/apps/visualize/_tsvb_chart·ts.visualize app visual builder "before each" hook for "should be able to switch between index patterns"Standard OutStack TraceBuild metrics
History
To update your PR or re-run it, just comment with: |
skh
added a commit
to skh/kibana
that referenced
this pull request
Jul 8, 2020
* Add usage collector for telemetry. * Make minimal usage collector work. * Add all fields to Usage and schema * Type packages as array. * Temporarily remove schema. * Temporarily exclude our collector from schema checks. * Add fleet telemetry. * Remove events from agent stats. * Add package telemetry. * Use correct import. * Add telemetry about enabled packages. * Clean up comments. * Update x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com> * Update x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com> * Correctly check for element in array. * Use a real SavedObjectsClient. * Remove useless use of undefined. * Use less deep path to import SavedObjectsClient. Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com> Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com>
skh
added a commit
that referenced
this pull request
Jul 8, 2020
* Add usage collector for telemetry. * Make minimal usage collector work. * Add all fields to Usage and schema * Type packages as array. * Temporarily remove schema. * Temporarily exclude our collector from schema checks. * Add fleet telemetry. * Remove events from agent stats. * Add package telemetry. * Use correct import. * Add telemetry about enabled packages. * Clean up comments. * Update x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com> * Update x-pack/plugins/ingest_manager/server/collectors/package_collectors.ts Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com> * Correctly check for element in array. * Use a real SavedObjectsClient. * Remove useless use of undefined. * Use less deep path to import SavedObjectsClient. Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com> Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com> Co-authored-by: Alejandro Fernández Haro <afharo@gmail.com> Co-authored-by: Nicolas Chaulet <n.chaulet@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #69282
This adds and registers a
usageCollectorfor the Ingest Manager plugin according to the telemetry documentation atHow to test this
Run a local setup with Ingest Manager enabled and log in to Kibana once, so that the initial setup of Ingest Manager has been performed.
Trigger a telemetry collection by visiting http://localhost:5601/api/stats?extended=true . In the output, find the
ingest_managerkey. All telemetry reported by this PR appears in the JSON object belonging to this key. (Some way to pretty-format JSON output is probably helpful, I'm using JSNView for Chrome.)xpack.ingestManager.fleet.enabled: falseinkibana.dev.ymlor any other means of configuring Kibana. This should result infleet_enabled: falsein the telemetry output. Enable fleet again and verify that you now seefleet_enabled: trueagentspart of the response should go up correspondingly.packagespart of the response should list thesystempackage withenabled: trueand theendpointpackage withenabled: false.Known issues
I had to do some wild typecasting in https://github.com/elastic/kibana/pull/69294/files#diff-02006980a036f05fecfa9277bb5a1c01R12 in order to use our existing methods to query saved objects. The issue is that according to the existing example code, accessing saved objects from the telemetry collector is done with the return value ofSOLVED, see discussion.core.savedObjects.createInternalRepository(), which for all practical purposes behaves like aSavedObjectsClient, but the types don't match up.Accesses to saved objects aren't paginated right now. This code catches and analyses the first
1000agent configs and the first20packages, mirroring what we do elsewhere in Ingest Manager. This is probably instantaneous tech debt -- I can amend this but wanted to open the PR for review now.