Skip to content
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

[APM] Service overview: Instances table #85770

Merged
merged 12 commits into from
Dec 15, 2020

Conversation

dgieselaar
Copy link
Member

Closes #81721.

image

@dgieselaar dgieselaar requested a review from a team as a code owner December 14, 2020 13:06
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Dec 14, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui (Team:apm)

Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great.

It would be very cool if you also removed the box for the latency distribution chart and made this full-width.

: serviceNodeName;

const link =
agentName === 'java' ? (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do have an isJavaAgentName function (it does the same thing but returns agentName is 'java' in common/agent_name.ts.

<EuiTitle size="xs">
<h2>
{i18n.translate('xpack.apm.serviceOverview.instancesTableTitle', {
defaultMessage: 'All instances',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this matches the mockup, but why is it not just "Instances"? @formgeist

),
render: (_, item) => {
const { serviceNodeName } = item;
const isMissing = serviceNodeName === SERVICE_NODE_NAME_MISSING;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
const isMissing = serviceNodeName === SERVICE_NODE_NAME_MISSING;
const isMissingNodeName = serviceNodeName === SERVICE_NODE_NAME_MISSING;

width: px(unit * 10),
render: (_, { throughput }) => {
return (
<SparkPlotWithValueLabel
Copy link
Member

@sorenlouv sorenlouv Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tangential: SparkPlotWithValueLabel seems like it should simply be:

<SparkPlot />

Then, if we want to hide the label value:

<SparkPlot showValueLabel={false} />

or perhaps:

<SparkPlot hideValueLabel={true} />


interface Response {
status: number;
body: APIReturnType<'GET /api/apm/services/{serviceName}/service_overview_instances'>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you not the slightest tempted to write a client when you did this? :p

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am 😬 but I would like someone else to pick that up.

Copy link
Member

@sorenlouv sorenlouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

) : (
<MetricOverviewLink
serviceName={serviceName}
mergeQuery={(query) => ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of mergeQuery can we just update MetricOverviewLink to handle the merge:

    <APMLink
      path={`/services/${serviceName}/metrics`}
      query={{
        ...query,
        ...pickKeys(urlParams as APMQueryParams, ...persistedFilters)
      }}
      {...rest}
    />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added mergeQuery to APMLink so it's reusable. I've also made it a function to leave ordering etc up to the consumer. One might want to set a default value, or override a parameter, or simply remove one.

@dgieselaar dgieselaar merged commit 8ccad31 into elastic:master Dec 15, 2020
@dgieselaar dgieselaar deleted the service-overview-instances-table branch December 15, 2020 10:04
dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Dec 15, 2020
gmmorris added a commit to ymao1/kibana that referenced this pull request Dec 15, 2020
* master: (66 commits)
  [Alerting] fixes broken Alerting Example plugin (elastic#85774)
  [APM] Service overview instances table (elastic#85770)
  [Security Solution] Unskip timeline creation Cypress test (elastic#85871)
  properly recognize enterprise licenses (elastic#85849)
  [SecuritySolution][Detections] Adds SavedObject persistence to Signals Migrations (elastic#85690)
  [TSVB] Fix functional tests flakiness and unskip them (elastic#85388)
  [Fleet] Change permissions for Fleet enroll role (elastic#85802)
  Gauge visualization can no longer be clicked to filter on values since Kibana 7.10.0 (elastic#84768)
  [Security Solution][Detections] Add alert source to detection rule action context (elastic#85488)
  [Discover] Don't display hide/show button for histogram when there's no time filter (elastic#85424)
  skip flaky suite (elastic#78553)
  License checks for alerts plugin (elastic#85649)
  skip flaky suite (elastic#84992)
  skip 'query return results valid for scripted field' elastic#78553
  Allow action types to perform their own mustache variable escaping in parameter templates (elastic#83919)
  [ML] More machine learning links in doc_links_service.ts (elastic#85365)
  Removed Alerting & Event Log deprecated fields that should not be using (elastic#85652)
  Closes elastic#79995 by adding new tab in transaction details to show related trace logs. (elastic#85859)
  Fix outdated jest snapshot
  [Maps] Surface on prem EMS (elastic#85729)
  ...
dgieselaar added a commit that referenced this pull request Dec 15, 2020
@sorenlouv sorenlouv changed the title [APM] Service overview instances table [APM] Service overview: Instances table Dec 18, 2020
@sorenlouv sorenlouv mentioned this pull request Dec 18, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 5.4MB 5.4MB +6.6KB

Distributable file count

id before after diff
default 47130 47893 +763

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team:APM All issues that need APM UI Team support v7.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Service overview: instances table
5 participants