-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Service map backend links #107317
Service map backend links #107317
Conversation
Pinging @elastic/apm-ui (Team:apm) |
@elasticmachine run the e2e |
x-pack/plugins/apm/public/components/app/service_map/Popover/backend_contents.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/public/components/app/service_map/Popover/backend_contents.tsx
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.
Link to backend detail page is broken
if (selectedNodeData[SPAN_TYPE] === 'resource') { | ||
return ResourceContents; | ||
} |
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 the RUM agent(s) fall-back? Should we exclude by agent rather than by span 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.
If it's an HTTP request (external/http) it could be called by a RUM agent or any other service, so we could be excluding links that do have backend data.
I suppose we could also make it so these do have stats and links, but then we would have the asymmetry of them being excluded from the dependencies table for RUM services.
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.
not sure I follow, we exclude all RUM calls, regardless of span types in the backend views
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.
The node data on the client does not have the agent name of the service it was called by, so we would have to make some modifications on the server to get this.
The node data available looks like:
{
id: ">sqlite"
label: "sqlite"
span.destination.service.resource: "sqlite"
span.subtype: "sqlite"
span.type: "db"
}
So I think looking for resource
is a reasonable way to do this for now.
x-pack/plugins/apm/public/components/app/service_map/Popover/backend_contents.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/apm/server/lib/service_map/get_service_map_backend_node_info.ts
Outdated
Show resolved
Hide resolved
re:
I think this will (should) be fixed by elastic/apm#443 |
import { StatsList } from './stats_list'; | ||
|
||
export function BackendContents({ nodeData }: ContentsProps) { | ||
const { query } = useApmParams('/*'); |
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.
if I do useApmParams('/*');
, then when I call apmRouter.link
below with query
I get:
info x-pack/plugins/apm/public/components/app/service_map/Popover/backend_contents.tsx:53:5 - error TS2322: Type '{} | { pageStep?: AgentConfigurationPageStep | undefined; } | { name?: string | undefined; environment?: string | undefined; pageStep?: AgentConfigurationPageStep | undefined; } | ... 5 more ... | ({ ...; } & { ...; })' is not assignable to type '({ rangeFrom?: string | undefined; rangeTo?: string | undefined; environment?: string | undefined; kuery?: string | undefined; } & { comparisonEnabled?: string | undefined; comparisonType?: string | undefined; }) | undefined'.
Type '{ pageStep?: AgentConfigurationPageStep | undefined; }' has no properties in common with type '{ rangeFrom?: string | undefined; rangeTo?: string | undefined; environment?: string | undefined; kuery?: string | undefined; } & { comparisonEnabled?: string | undefined; comparisonType?: string | undefined; }'.
53 query,
~~~~~
x-pack/plugins/apm/public/components/routing/home/index.tsx:57:5
57 query: t.partial({
~~~~~~~~~~~~~~~~~~
58 rangeFrom: t.string,
~~~~~~~~~~~~~~~~~~~~~~~~~~
...
61 kuery: t.string,
~~~~~~~~~~~~~~~~~~~~~~
62 }),
~~~~~~
The expected type comes from property 'query' which is declared here on type '{ query?: { rangeFrom?: string | undefined; rangeTo?: string | undefined; environment?: string | undefined; kuery?: string | undefined; } | undefined; } & { query?: { comparisonEnabled?: string | undefined; comparisonType?: string | undefined; } | undefined; } & { ...; }'
Since the only think I'm using query
for is to pass into link
below should I just do useApmParams('/backends/:backendName/overview')
here? Or just as
up the call below?
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.
Nevermind I can't do useApmParams('/backends/:backendName/overview')
, because then I get no matching route
.
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 just used query as any
below
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.
...or query as TypeOf< ApmRoutes, '/backends/:backendName/overview' >['query'],
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (392 commits) update linting doc (elastic#105748) [APM] Various improvements from elastic#104851 (elastic#107726) Update dependency @elastic/charts to v33.2.0 (master) (elastic#107842) Fix default route link on kibana homepage (elastic#107809) [APM] Invalidate trackPageview on route change (elastic#107741) Service map backend links (elastic#107317) [index patterns] index pattern create modal (elastic#101853) [RAC] integrating rbac search strategy with alert table (elastic#107242) [Security Solution] Siem signals -> alerts as data field and index aliases (elastic#106049) [Metrics UI] Add checkbox to optionally drop partial buckets (elastic#107676) [Metrics UI] Fix metric threshold preview regression (elastic#107674) Disable Product check in @elastic/elasticsearch-js (elastic#107642) [App Search] Migrate Crawler Status Indicator, Crawler Status Banner, and Crawl Request polling (elastic#107603) [Security Solution, Lists] Replace legacy imports from 'elasticsearch' package (elastic#107226) [maps] asset tracking tutorial (elastic#104552) [scripts/build_ts_refs] when using `--clean` initialize caches (elastic#107777) Upgrade EUI to v36.1.0 (elastic#107231) [RAC] [TGrid] Implements cell actions in the TGrid (elastic#107771) Realign cypress/ccs_integration with cypress/integration (elastic#107743) Allow optional OSS to X-Pack dependencies (elastic#107432) ... # Conflicts: # x-pack/examples/reporting_example/public/application.tsx # x-pack/examples/reporting_example/public/components/app.tsx # x-pack/plugins/canvas/public/services/legacy/stubs/reporting.ts # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/lib/reporting_api_client/context.tsx # x-pack/plugins/reporting/public/management/mount_management_section.tsx # x-pack/plugins/reporting/public/management/report_listing.test.tsx # x-pack/plugins/reporting/public/plugin.ts # x-pack/plugins/reporting/public/share_context_menu/register_pdf_png_reporting.tsx # x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts
Add links to backends on service map popovers.
Includes a lot of refactoring of these, but functionally the only difference should be that the backends now show a link to the backends page and stats.
Make the redis icon show up for cache/redis in addition to db/redis.
Fixes #103259