-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources #204026
[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources #204026
Conversation
…se_discover_redirect.ts Co-authored-by: Carlos Crespo <[email protected]>
…se_discover_redirect.ts Co-authored-by: Carlos Crespo <[email protected]>
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
...s/observability_solution/inventory/server/routes/entity_definition/get_entity_definitions.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/inventory/public/hooks/use_discover_redirect.ts
Outdated
Show resolved
Hide resolved
...s/observability_solution/inventory/server/routes/entity_definition/get_entity_definitions.ts
Outdated
Show resolved
Hide resolved
entity.entityDefinitionId | ||
); | ||
const { entityDefinitionIndexPatterns, isEntityDefinitionIndexPatternsLoading } = | ||
useFetchEntityDefinitionIndexPattern(entity.entityType); |
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.
Entities of the same type are expected to have the same index pattern, right?
Calling useFetchEntityDefinitionIndexPattern
in use_discover_redirect
means that for each row a request to inventory/entity/definitions/sources?type={type}
will be triggered.
What do you think of sending the request only when the panel is expanded? Alternatively, we could fetch the existing entity types, such as services and hosts, in a single request:
and request inventory/entity/definitions/sources?types=services,hosts
This could return a response like:
definitionIndexPatterns: {
"services": "metrics-apm*, logs-apm*",
"hosts": "metrics-infra*"
}
This would reduce redundant requests and improve scalability. What do you think?
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.
That's a good idea! I would totally move the fetching outside of the hook to a higher level and use it once per type.
Alternatively, we could fetch the existing entity types, such as services and hosts, in a single request:
and request inventory/entity/definitions/sources?types=services,hosts
This could return a response like:
definitionIndexPatterns: {
"services": "metrics-apm*, logs-apm*",
"hosts": "metrics-infra*"
}
I tried to do it as suggested with multiple types (here) but the EEM API expects a single type so even if we combine our API response we will still need to call the EEM API several times which is not necessary for the types before we expand the group (Which will make requests to EEM which are not needed at this point as some of the groups are not opened)
What do you think of sending the request only when the panel is expanded?
I would prefer that option rather than the combined response for multiple types and get only the index patterns for the selected group by it's type
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.
EEM API expects a single type
As the main consumers of the API, we have the privilege of requesting an extension if needed. 😊
I would prefer that option rather than the combined response for multiple types and get only the index patterns for the selected group by it's type
sounds good to me.
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 moved the logic to get the index patterns to the grouped_entities_grid.tsx and passed it down to the grid so we will do one request per group. Currently, it's tricky to test without the changes from #203452 I will retest it once we merge 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.
Update: I ended up changing to multiple types having the type_id as key and index patterns as value like:
{
built_in_hosts_from_ecs_data: [ 'filebeat-*', 'logs-*', 'metrics-*', 'metricbeat-*' ]
}
EEM API expects a single type
As the main consumers of the API, we have the privilege of requesting an extension if needed. 😊
I tried first to use what we have but I will comment on the issue based on my findings here to request that 😁
…ntory-v2-adhoc-data-view
return { | ||
definitionIndexPatterns: { | ||
...Object.fromEntries( | ||
Array.from(new Set(allEntityDefinitionIndexPatterns)).flatMap(Object.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.
Is this necessary? Since the array is already structured in line 24, couldn't be formatted there?
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 don't think I understand the issue. The idea of this is to format the data in a format where all the unique values are extracted and converted to an object like: { unique type name
: [ all unique array values (without duplicates)] } - the mapping will return an array were here we want to get the unique key/values.
For context: we can have more than one source per type (comment) so from the sources we can have for example: type n
with 2 different index pattern arrays: [index1], [index2] ... and [indexN] and we want to transform [{n: [index1]},....] to {n: [index1, index2....]} - that's why we need the set conversion/flatmap conversion of the allEntityDefinitionIndexPatterns
Does it make sense now?
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.
Update: Based on the suggestions I see that that wasn't super clear so I refactored that part and added a test for it in this commit 68d076c
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 didn't know that one type can have 1 or n sources. Thanks 💯
@@ -24,32 +24,28 @@ export const EntityActions = ({ entity, setShowActions }: Props) => { | |||
? `inventoryEntityActionsButton-${entity.entityDisplayName}` | |||
: 'inventoryEntityActionsButton'; | |||
|
|||
const { getDiscoverEntitiesRedirectUrl, isEntityDefinitionLoading } = useDiscoverRedirect(entity); | |||
const { getDiscoverEntitiesRedirectUrl } = useDiscoverRedirect(entity); |
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 noticed that the ad hoc data view is created for each row causing redundant requests
Screen.Recording.2025-01-13.at.13.20.38.mov
Could you please take a look
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.
Thanks for spotting that! I missed that after the refactoring - so I moved the discover link dataview to the context and set the entity type once the grid is opened so now it should work as expected:
discover_link_fix.mov
Can you please check again?
...ns/observability/plugins/inventory/server/routes/entity_definition/get_entity_definitions.ts
Outdated
Show resolved
Hide resolved
.../inventory/server/routes/entity_definition/extract_entity_index_patterns_from_definitions.ts
Outdated
Show resolved
Hide resolved
5afc869
to
03f968b
Compare
03f968b
to
1f03ce0
Compare
import type { EntitySourceDefinition } from '@kbn/entityManager-plugin/server/lib/v2/types'; | ||
import { extractEntityIndexPatternsFromDefinitions } from './extract_entity_index_patterns_from_definitions'; | ||
|
||
describe('extractEntityIndexPatternsFromDefinitions', () => { |
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.
❤️
x-pack/solutions/observability/plugins/inventory/public/hooks/use_discover_redirect.ts
Outdated
Show resolved
Hide resolved
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.
Code LGTM, thanks for addressing the comments.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Async chunks
History
|
Starting backport for target branches: 8.x |
…point using sources (elastic#204026) Closes elastic#202298 This PR changes the way we get the entity index patterns to v2. It creates an endpoint part of the inventory API which returns the index patterns by entity type. ## Testing ### Test the endpoint: - Open Dev tools and add ` GET kbn:/internal/inventory/entity/definitions/sources` - Response: ![image](https://github.com/user-attachments/assets/3346c36e-dbc2-4e56-9ed6-d3d3a8f7d1a5) ### Test in the UI - After the previous steps add some host data (oblt cluster / metricbeat) or use synthtrace (for example use `node scripts/synthtrace infra_hosts_with_apm_hosts --scenarioOpts.numInstances=10` or `node scripts/synthtrace logs_traces_hosts.ts`) - Go to Inventory and expand the host group - Click on the actions button for any host and click on the Discover link - The correct dataview should be selected based on the index patterns in the source definition The same can be done for other entity types - Test the search bar as well (the suggestions should be visible) and now we should have 1 request to get the sources (instead of doing it on click) https://github.com/user-attachments/assets/93b5ac6c-9d64-44e0-b26e-6133477e0840 <!--ONMERGE {"backportTargets":["8.x"]} ONMERGE--> --------- Co-authored-by: Carlos Crespo <[email protected]> Co-authored-by: Sergi Romeu <[email protected]> Co-authored-by: kibanamachine <[email protected]> (cherry picked from commit ffccfdc)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…on endpoint using sources (#204026) (#206610) # Backport This will backport the following commits from `main` to `8.x`: - [[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources (#204026)](#204026) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"jennypavlova","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-01-14T15:30:39Z","message":"[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources (#204026)\n\nCloses #202298 \r\n\r\nThis PR changes the way we get the entity index patterns to v2. It\r\ncreates an endpoint part of the inventory API which returns the index\r\npatterns by entity type.\r\n\r\n## Testing\r\n\r\n### Test the endpoint: \r\n- Open Dev tools and add\r\n` GET kbn:/internal/inventory/entity/definitions/sources`\r\n- Response: \r\n\r\n\r\n![image](https://github.com/user-attachments/assets/3346c36e-dbc2-4e56-9ed6-d3d3a8f7d1a5)\r\n\r\n\r\n### Test in the UI\r\n- After the previous steps add some host data (oblt cluster /\r\nmetricbeat) or use synthtrace (for example use `node scripts/synthtrace\r\ninfra_hosts_with_apm_hosts --scenarioOpts.numInstances=10` or `node\r\nscripts/synthtrace logs_traces_hosts.ts`)\r\n- Go to Inventory and expand the host group\r\n- Click on the actions button for any host and click on the Discover\r\nlink\r\n- The correct dataview should be selected based on the index patterns in\r\nthe source definition\r\nThe same can be done for other entity types\r\n- Test the search bar as well (the suggestions should be visible) and\r\nnow we should have 1 request to get the sources (instead of doing it on\r\nclick)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93b5ac6c-9d64-44e0-b26e-6133477e0840\r\n\r\n\r\n\r\n\r\n<!--ONMERGE {\"backportTargets\":[\"8.x\"]} ONMERGE-->\r\n\r\n---------\r\n\r\nCo-authored-by: Carlos Crespo <[email protected]>\r\nCo-authored-by: Sergi Romeu <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ffccfdc62cd1da5baf54568cbee20a9a3466178e","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","backport:prev-minor","ci:project-deploy-observability","Team:obs-ux-infra_services"],"title":"[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources","number":204026,"url":"https://github.com/elastic/kibana/pull/204026","mergeCommit":{"message":"[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources (#204026)\n\nCloses #202298 \r\n\r\nThis PR changes the way we get the entity index patterns to v2. It\r\ncreates an endpoint part of the inventory API which returns the index\r\npatterns by entity type.\r\n\r\n## Testing\r\n\r\n### Test the endpoint: \r\n- Open Dev tools and add\r\n` GET kbn:/internal/inventory/entity/definitions/sources`\r\n- Response: \r\n\r\n\r\n![image](https://github.com/user-attachments/assets/3346c36e-dbc2-4e56-9ed6-d3d3a8f7d1a5)\r\n\r\n\r\n### Test in the UI\r\n- After the previous steps add some host data (oblt cluster /\r\nmetricbeat) or use synthtrace (for example use `node scripts/synthtrace\r\ninfra_hosts_with_apm_hosts --scenarioOpts.numInstances=10` or `node\r\nscripts/synthtrace logs_traces_hosts.ts`)\r\n- Go to Inventory and expand the host group\r\n- Click on the actions button for any host and click on the Discover\r\nlink\r\n- The correct dataview should be selected based on the index patterns in\r\nthe source definition\r\nThe same can be done for other entity types\r\n- Test the search bar as well (the suggestions should be visible) and\r\nnow we should have 1 request to get the sources (instead of doing it on\r\nclick)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93b5ac6c-9d64-44e0-b26e-6133477e0840\r\n\r\n\r\n\r\n\r\n<!--ONMERGE {\"backportTargets\":[\"8.x\"]} ONMERGE-->\r\n\r\n---------\r\n\r\nCo-authored-by: Carlos Crespo <[email protected]>\r\nCo-authored-by: Sergi Romeu <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ffccfdc62cd1da5baf54568cbee20a9a3466178e"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/204026","number":204026,"mergeCommit":{"message":"[ECO][Inventory v2] Ad hoc data view: Add get entities definition endpoint using sources (#204026)\n\nCloses #202298 \r\n\r\nThis PR changes the way we get the entity index patterns to v2. It\r\ncreates an endpoint part of the inventory API which returns the index\r\npatterns by entity type.\r\n\r\n## Testing\r\n\r\n### Test the endpoint: \r\n- Open Dev tools and add\r\n` GET kbn:/internal/inventory/entity/definitions/sources`\r\n- Response: \r\n\r\n\r\n![image](https://github.com/user-attachments/assets/3346c36e-dbc2-4e56-9ed6-d3d3a8f7d1a5)\r\n\r\n\r\n### Test in the UI\r\n- After the previous steps add some host data (oblt cluster /\r\nmetricbeat) or use synthtrace (for example use `node scripts/synthtrace\r\ninfra_hosts_with_apm_hosts --scenarioOpts.numInstances=10` or `node\r\nscripts/synthtrace logs_traces_hosts.ts`)\r\n- Go to Inventory and expand the host group\r\n- Click on the actions button for any host and click on the Discover\r\nlink\r\n- The correct dataview should be selected based on the index patterns in\r\nthe source definition\r\nThe same can be done for other entity types\r\n- Test the search bar as well (the suggestions should be visible) and\r\nnow we should have 1 request to get the sources (instead of doing it on\r\nclick)\r\n\r\n\r\n\r\nhttps://github.com/user-attachments/assets/93b5ac6c-9d64-44e0-b26e-6133477e0840\r\n\r\n\r\n\r\n\r\n<!--ONMERGE {\"backportTargets\":[\"8.x\"]} ONMERGE-->\r\n\r\n---------\r\n\r\nCo-authored-by: Carlos Crespo <[email protected]>\r\nCo-authored-by: Sergi Romeu <[email protected]>\r\nCo-authored-by: kibanamachine <[email protected]>","sha":"ffccfdc62cd1da5baf54568cbee20a9a3466178e"}}]}] BACKPORT--> Co-authored-by: jennypavlova <[email protected]>
@@ -37,19 +24,19 @@ export const useDiscoverRedirect = (entity: InventoryEntity) => { | |||
}) | |||
: ''; | |||
|
|||
return application.capabilities.discover?.show | |||
return application.capabilities.discover?.show || !dataView |
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.
@jennypavlova Sorry to show up on this PR after it's already merged, but I just encountered this line when resolving a merge conflict and thought it was worth verifying. If I'm reading this right, a Discover link will display if the user has the show
capability or if there is no dataView
. But couldn't the || !dataView
here cause the link to show in some cases even when the user doesn't have the required Discover capability?
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.
Thank you for the comment, good catch! I think we can remove the || !dataView
or make it an AND
condition - this can be addressed as part of #207064 as it will be touching this part.
Closes #202298
This PR changes the way we get the entity index patterns to v2. It creates an endpoint part of the inventory API which returns the index patterns by entity type.
Testing
Test the endpoint:
GET kbn:/internal/inventory/entity/definitions/sources
Test in the UI
node scripts/synthtrace infra_hosts_with_apm_hosts --scenarioOpts.numInstances=10
ornode scripts/synthtrace logs_traces_hosts.ts
)The same can be done for other entity types
Link_Search_Test.mov