Update types, schemas, and saved objects for datasources. #58921
Conversation
This file contains 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
Based on specs/examples at https://github.com/elastic/beats/blob/feature-ingest/x-pack/agent/docs/agent_configuration_example.yml#L28-L61 & https://github.com/elastic/beats/blob/c46ae67648b08f8d263de3eb7db870f77bbc2558/x-pack/agent/pkg/agent/program/testdata/single_config.yml#L22-L40 Create & list for /datasources API worked as expected Fails type checks because of issues on one UI view (datasources table): ``` ERROR x-pack failed x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/details_page/components/datasources_table.tsx:46:37 - error TS2339: Property 'name' does not exist on type 'DatasourceWithConfig'. 46 originalDatasources?.map(({ id, name, inputs, package: datasourcePackage, configs }) => ({ ~~~~ x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/details_page/components/datasources_table.tsx:51:40 - error TS2339: Property 'title' does not exist on type '{ name: string; version: string; }'. 51 packageTitle: datasourcePackage?.title, ~~~~~ x-pack/plugins/ingest_manager/public/applications/ingest_manager/sections/agent_config/details_page/components/datasources_table.tsx:53:46 - error TS2339: Property 'description' does not exist on type '{ name: string; version: string; }'. 53 packageDescription: datasourcePackage?.description, ~~~~~~~~~~~ Found 3 errors. ```
|
For the datasource table, the new one changed a lot I think we can use id instead of name see proto |
💔 Build FailedTest FailuresKibana Pipeline / kibana-oss-agent / Chrome UI Functional Tests.test/functional/apps/context/_date_nanos·js.context app context view for date_nanos displays predessors - anchor - successors in right orderStandard OutStack TraceTo update your PR or re-run it, just comment with: |
| enabled?: boolean; | ||
| dataset?: string; | ||
| metricset?: string; | ||
| paths?: string[]; |
There was a problem hiding this comment.
Question: don't we need something like config (from prior version) that accepts free-typed JSON?
I'm thinking of the use case for Endpoint where we will need to add our policy configuration data.
The example in the beats repo has two suggestions for this use case (line 472 and 578 - I thought that from one of the review meetings, the Endpoint team (@ferullo ) indicated suggestion 2 was best but @ph can confirm.
michalpristas
approved these changes
Mar 2, 2020
jen-huang
requested changes
Mar 2, 2020
There was a problem hiding this comment.
I do not think this PR is needed. Our saved object schema does not, and should not, match the agent config spec exactly. The saved object schema needs to 1) provide enough information for UI usage and 2) contain enough fields to translate to agent config. There will be a business layer that translates the saved object to agent config. My work in #58670 referenced the agent config spec, and contains changes to datasources and output types.
I left some comments for explanation. Please let me know if you disagree or see anything that we do need to change that isn't covered by the previous work.
Comment on lines
+23
to
+24
| metricset?: string; | ||
| paths?: string[]; |
There was a problem hiding this comment.
There are examples of fields that don't need to exist in the saved object schema. metricset and paths are not applicable to all streams, that's why we have a config property for arbitrary stream config fields that will then be flattened into the top-level streams object at the business logic layer of converting SO -> agent config.
There was a problem hiding this comment.
Yeah, I was wondering if this wasn't perhaps an A | B | C type and if we needed to be that involved in the Agent configs
| version: string; | ||
| }; | ||
| output_id: string; | ||
| namespace?: string; | ||
| use_output: string; |
There was a problem hiding this comment.
This is an example where it's ok that the field name differs in SO than agent schema. The field names will be remapped, but the SO name is more friendly to work with in UI.
|
@jen-huang 🤷♀ I hear what you're saying. As I said, I'm not completely clear here. In the Thursday meeting, @ph mentioned some Fleet changes related to the new Agent config. I offered to help and to sync up with @nchaulet. We met and this is what I understood the changes to be. I don't know if that work is still needed elsewhere or if the changes you mentioned make it unnecessary. Just let me know how I can help. |
ruflin
added
Feature:EPM
|
Pinging @elastic/ingest (Feature:EPM) |
jen-huang
added
the
Team:Fleet
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
refs #58874
Based on specs/examples from beats repo 1 & 2
👍 /datasources POST accepted the various examples linked above and GET returned the list of the datasource objects
❓ Is this correct? If so, what else do I need to do? Add/update tests? Any esarchiver data/mappings ?
👎Fails type checks because of issues on one UI view (datasources table):