[APM] Average latency map for mobile service overview #144127
Conversation
gbamparop
changed the title
|
Pinging @elastic/apm-ui (Team:APM) |
gbamparop
added
the
release_note:skip
| ), | ||
| }, | ||
| ], | ||
| indexPatternId: APM_STATIC_DATA_VIEW_ID, |
There was a problem hiding this comment.
Ad hoc data views could be used as a follow up
| import { useApmParams } from '../../../../../hooks/use_apm_params'; | ||
| import { useTimeRange } from '../../../../../hooks/use_time_range'; | ||
|
|
||
| export function EmbeddedMapComponent({ filters }: { filters: Filter[] }) { |
There was a problem hiding this comment.
Do you need to export this component?
There was a problem hiding this comment.
Good spot, removed the export
| | EmbeddableFactory< | ||
| MapEmbeddableInput, | ||
| MapEmbeddableOutput, | ||
| MapEmbeddable | ||
| > |
There was a problem hiding this comment.
Shouldn't this type be inferred by TS based on the response of embeddablePlugin?.getEmbeddableFactory?
There was a problem hiding this comment.
It's not inferred, but I've moved it to the call of getEmbeddableFactory
| if (!factory) { | ||
| setError(true); | ||
| notifications?.toasts.addDanger({ | ||
| title: i18n.translate( | ||
| 'xpack.apm.serviceOverview.embeddedMap.error.toastTitle', | ||
| { | ||
| defaultMessage: 'An error occurred when adding map embeddable', | ||
| } | ||
| ), | ||
| text: i18n.translate( | ||
| 'xpack.apm.serviceOverview.embeddedMap.error.toastDescription', | ||
| { | ||
| defaultMessage: `Embeddable factory with id "{embeddableFactoryId}" was not found.`, | ||
| values: { | ||
| embeddableFactoryId: MAP_SAVED_OBJECT_TYPE, | ||
| }, | ||
| } | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Can't you stop the execution of this function if the factory is not defined?
| hideFilterActions: true, | ||
| }; | ||
|
|
||
| const embeddableObject = await factory?.create(input); |
There was a problem hiding this comment.
Like the comment above, you wouldn't need to add ? after factory here if you had stoped the function or only run this peace of code in the else block when factory is defined.
| SERVICE_VERSION, | ||
| } from '../../../../../common/elasticsearch_fieldnames'; | ||
|
|
||
| function termQuery<T extends string>( |
There was a problem hiding this comment.
Can't you use the one that already exists in the Observability plugin
There was a problem hiding this comment.
It's in the /server, the plan is to move it to a common folder on a follow up PR. (#144304)
| const { | ||
| path: { serviceName }, | ||
| query: { | ||
| environment, | ||
| transactionType, | ||
| device, | ||
| osVersion, | ||
| appVersion, | ||
| netConnectionType, | ||
| }, | ||
| } = useApmParams('/services/{serviceName}/overview'); |
There was a problem hiding this comment.
Don't you feel it would be better if this function receives as parameters all these values instead of using useApmParams? If we decide to use this on another page we'd have to add another path here and so on.
There was a problem hiding this comment.
I think it's more clear this way to just return the filters, but agree if we find out that it is being used in other pages and have to add more and more we can refactor.
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Add average latency map to mobile service overview.
Screen.Recording.2022-10-31.at.18.59.11.mov
Closes #143502