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] Various improvements from #104851 #107726

Merged
merged 10 commits into from Aug 7, 2021

Conversation

sorenlouv
Copy link
Member

In #104851 I wanted to fix the throughput charts. While doing this I ran into a couple of issues with our types that made the change harder.

This PR doesn't contain any user facing changes (also no changes to how throughput is being calculated). It only contains various minor improvements to our types that should make changes slightly easier in the future.

@sorenlouv sorenlouv requested a review from a team as a code owner August 5, 2021 09:23
@botelastic botelastic bot added the Team:APM All issues that need APM UI Team support label Aug 5, 2021
@elasticmachine
Copy link
Contributor

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

export interface KeyValue {
key: string;
value: any | undefined;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused

/**
* timestamp in milliseconds or ISO timestamp
*/
time: number | string,
Copy link
Member Author

@sorenlouv sorenlouv Aug 5, 2021

Choose a reason for hiding this comment

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

Turns out that we actually call this function with string but due to manual type overrides we've newer noticed.

@@ -21,6 +21,7 @@ export interface ITableColumn<T> {
align?: string;
width?: string;
sortable?: boolean;
truncateText?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

is this being used somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, in

We didn't have typed table columns before so it wasn't an error until now

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for doing this

@@ -127,10 +127,10 @@ export function AgentConfigurationList({
width: theme.eui.euiSizeXL,
name: '',
sortable: true,
render: (isApplied: boolean) => (
render: (_, { applied_by_agent: appliedByAgent }) => (
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding manual type overrides and preserving the correct type

@@ -31,7 +31,7 @@ export function CustomLinkTable({ items = [], onCustomLinkSelected }: Props) {
const { core } = useApmPluginContext();
const canSave = core.application.capabilities.apm.save;

const columns = [
const columns: Array<ITableColumn<CustomLink>> = [
Copy link
Member Author

@sorenlouv sorenlouv Aug 5, 2021

Choose a reason for hiding this comment

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

Adding types to table columns

latency: number;
cpuUsage: number;
memoryUsage: number;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid explicit types when the (correct) type can be infered via APIReturnType

@sorenlouv sorenlouv added auto-backport Deprecated: Automatically backport this PR after it's merged release_note:skip Skip the PR/issue when compiling release notes v7.14.0 labels Aug 5, 2021
@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 4.3MB 4.3MB +382.0B
Unknown metric groups

Non-exported public API item count

id before after diff
apm 30 29 -1

History

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

@sorenlouv
Copy link
Member Author

jenkins run the e2e

@sorenlouv sorenlouv merged commit b16d20b into elastic:master Aug 7, 2021
@sorenlouv sorenlouv deleted the followup-fixes-throughput-fixes branch August 7, 2021 11:22
@kibanamachine
Copy link
Contributor

The following labels were identified as gaps in your version labels and will be added automatically:

  • v7.15.0

If any of these should not be on your pull request, please manually remove them.

@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.14 Commit could not be cherrypicked due to conflicts
7.x

Successful backport PRs will be merged automatically after passing CI.

To backport manually run:
node scripts/backport --pr 107726

kibanamachine added a commit that referenced this pull request Aug 7, 2021
Co-authored-by: Søren Louv-Jansen <sorenlouv@gmail.com>
streamich pushed a commit to vadimkibana/kibana that referenced this pull request Aug 8, 2021
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 9, 2021
…-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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support v7.14.0 v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants