[APM] Display all properties by default in flyout #113221

dgieselaar · 2021-09-28T07:10:10Z

Closes #71985.

Use an exception list rather than a allowlist, and instead of using _source, use the _fields API to be able to get consistent output of values.

Closes elastic#71985.

…erties

elasticmachine · 2021-09-28T07:10:12Z

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

dgieselaar · 2021-09-28T07:13:30Z

@formgeist right now I'm capitalising all section labels. Previously we hardcoded some. So, what was previously HTTP is now Http, which looks kind of odd. Do you want to keep the list of hardcoded overrides, or maybe just lowercase them all?

formgeist · 2021-09-28T07:45:29Z

@formgeist right now I'm capitalising all section labels. Previously we hardcoded some. So, what was previously HTTP is now Http, which looks kind of odd. Do you want to keep the list of hardcoded overrides, or maybe just lowercase them all?

I think lowercasing them all makes sense 👍 Btw. will this resolve this issue I found in the backlog since we'll show all related metadata to the transaction?

dgieselaar · 2021-09-28T08:10:51Z

@formgeist:

will this resolve this issue I found in the backlog since we'll show all related metadata to the transaction?

not yet, as error.exception is excluded (similar to span.stacktrace). Should we just exclude error.exception.stacktrace? @sqren

formgeist · 2021-09-28T08:19:09Z

Should we just exclude error.exception.stacktrace?

Makes sense to exclude the stacktraces (I imagine they're the ones that are going to cause space issues).

mshustov · 2021-09-28T11:48:44Z

src/core/types/elasticsearch/search.ts

@@ -48,7 +48,7 @@ type ValueTypeOfField<T> = T extends Record<string, string | number>

 type MaybeArray<T> = T | T[];

-type Fields = Exclude<Required<estypes.SearchRequest>['body']['fields'], undefined>;
+type Fields = MaybeArray<Exclude<Required<estypes.SearchRequest>['body']['fields'], undefined>>;


Can't we remove a custom type and use estypes. Fields instead? It's it became exported recently.

I'll give it a shot - previously I think that type was incorrect (e.g. it didn't support arrays or something similar)

estypes.Fields is string | string[], but it can also be an array of objects, e.g. [{ field: '*', include_unmapped: true }]

It seems it's already supported?

SearchRequest.body.fields?: (Field | DateField)[]; export type Field = 'second' | 'minute' | 'hour' | 'day' | 'date' | 'month'; export interface DateField { field: Field format?: string include_unmapped?: boolean }

ah yeah, but that's not what is being exported estypes.Fields. I can remove the MaybeArray part though, that's not correct anyway. Thanks!

sorenlouv · 2021-09-28T13:12:03Z

not yet, as error.exception is excluded (similar to span.stacktrace). Should we just exclude error.exception.stacktrace? @sqren

sgtm unless this is very noisy.

smith

Code looks good. Only requesting changes because of the duplicate license header mentioned in a comment.

smith · 2021-09-29T16:49:20Z

x-pack/plugins/apm/public/components/shared/MetadataTable/MetadataTable.test.tsx

@@ -11,7 +11,7 @@ import { MemoryRouter } from 'react-router-dom';
 import { MetadataTable } from '.';
 import { MockApmPluginContextWrapper } from '../../../context/apm_plugin/mock_apm_plugin_context';
 import { expectTextsInDocument } from '../../../utils/testHelpers';
-import { SectionsWithRows } from './helper';
+import { SectionDescriptor } from './types';


Suggested change

import { SectionDescriptor } from './types';

import type { SectionDescriptor } from './types';

it's just a test so doesn't matter for bundle size, but still nice to do.

++, I think type imports are removed for jest tests as well, which helps with performance (with jest rewriting all the import statements)

smith · 2021-09-29T16:51:41Z

x-pack/plugins/apm/public/components/shared/MetadataTable/SpanMetadata/index.tsx

@@ -5,20 +5,49 @@
 * 2.0.
 */

+/*


Duplicate license header here.

smith · 2021-09-29T16:54:37Z

x-pack/test/apm_api_integration/tests/index.ts

@@ -37,6 +37,25 @@ export default function apmApiIntegrationTests(providerContext: FtrProviderConte
      loadTestFile(require.resolve('./correlations/latency'));
    });

+<<<<<<< HEAD


oops indeed

smith · 2021-09-29T17:37:04Z

Typing in the search filtering box on that panel seems to be very slow, like it's tying up the main thread for a second each time I type a character. It's not making network requests, and this is not something new in this PR, but this might be a good opportunity to improve the performance if it's not too difficult.

sorenlouv · 2021-09-29T22:40:16Z

Typing in the search filtering box on that panel seems to be very slow, like it's tying up the main thread for a second each time I type a character. It's not making network requests, and this is not something new in this PR, but this might be a good opportunity to improve the performance if it's not too difficult.

Can we add throttling to limit the number of updates?

sorenlouv · 2021-09-29T22:48:44Z

x-pack/test/apm_api_integration/tests/metadata/event_metadata.ts

+      },
+    });
+
+    return get(response.body.hits.hits[0]._source, `${processorEvent}.id`);


Do we need get?

Suggested change

return get(response.body.hits.hits[0]._source, `${processorEvent}.id`);

return response.body.hits.hits[0]._source[processorEvent].id;

or if optional:

Suggested change

return get(response.body.hits.hits[0]._source, `${processorEvent}.id`);

return response.body.hits.hits[0]._source?.[processorEvent]?.id;

sure, ts will complain out of the box but I can add a type for _source.

x-pack/plugins/apm/server/lib/event_metadata/get_event_metadata.ts

…erties

formgeist

Just a small comment on the section title capitalization.

formgeist · 2021-09-30T07:19:37Z

x-pack/plugins/apm/public/components/shared/MetadataTable/index.tsx

            <EuiTitle size="xs">
              <h6>{section.label}</h6>
            </EuiTitle>


I thought we'd force lowercase capitalization for the section titles so we'd avoid the odd-cases like Http > http which is true to the raw formatting in the fieldnames. I think it'll make it easier to scan.

yes, on my list!

dgieselaar · 2021-09-30T10:01:09Z

@smith @sqren I can't reproduce the slowness, what specific place are you looking at (transaction metadata tab, transaction/span flyout, error metadata tab)?

…erties

mshustov

ok for the core changes

x-pack/plugins/apm/public/components/shared/MetadataTable/SpanMetadata/index.tsx

dgieselaar · 2021-10-05T07:18:34Z

@elasticmachine merge upstream

kibanamachine · 2021-10-05T09:20:30Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1178	1174	-4

Async chunks

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

id	before	after	diff
`apm`	2.7MB	2.7MB	-1.1KB

History

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

kibanamachine · 2021-10-05T11:41:50Z

💔 Backport failed

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

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

kibanamachine · 2021-10-07T11:44:59Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113221 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-10-08T11:54:58Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113221 or prevent reminders by adding the backport:skip label.

kibanamachine · 2021-10-11T12:04:58Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 113221 or prevent reminders by adding the backport:skip label.

Co-authored-by: Kibana Machine <[email protected]> # Conflicts: # x-pack/plugins/apm/public/components/shared/MetadataTable/helper.ts

Co-authored-by: Kibana Machine <[email protected]>

dgieselaar added 3 commits September 23, 2021 12:29

[APM] Display all properties by default in flyout

eb937f2

Closes elastic#71985.

Labels first, exclude stacktrace

a2c2f8c

Merge branch 'master' of github.com:elastic/kibana into show-all-prop…

c1bf79f

…erties

dgieselaar added release_note:fix Team:APM All issues that need APM UI Team support auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Sep 28, 2021

dgieselaar requested review from a team as code owners September 28, 2021 07:10

mshustov reviewed Sep 28, 2021

View reviewed changes

smith requested changes Sep 29, 2021

View reviewed changes

sorenlouv reviewed Sep 29, 2021

View reviewed changes

Merge branch 'master' of github.com:elastic/kibana into show-all-prop…

a6cb2bf

…erties

formgeist reviewed Sep 30, 2021

View reviewed changes

Review feedback

9a6068c

dgieselaar added 3 commits September 30, 2021 12:16

More review feedback

2ba4404

Remove unused i18n labels

5845799

Merge branch 'master' of github.com:elastic/kibana into show-all-prop…

bb92b03

…erties

dgieselaar linked an issue Sep 30, 2021 that may be closed by this pull request

[APM] Show error.exception[0].attributes object in Metadata tab on Errors detail page #20664

Closed

dgieselaar added 2 commits September 30, 2021 13:21

Remove unused import

445d47a

Use estypes.SearchRequest.body.fields type

70778a2

dgieselaar requested a review from smith September 30, 2021 11:41

mshustov approved these changes Oct 1, 2021

View reviewed changes

sorenlouv reviewed Oct 1, 2021

View reviewed changes

x-pack/plugins/apm/public/components/shared/MetadataTable/SpanMetadata/index.tsx Show resolved Hide resolved

smith approved these changes Oct 4, 2021

View reviewed changes

sorenlouv approved these changes Oct 4, 2021

View reviewed changes

Merge branch 'master' into show-all-properties

3b7e96e

dgieselaar merged commit 79f841e into elastic:master Oct 5, 2021

dgieselaar deleted the show-all-properties branch October 5, 2021 11:38

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 7, 2021

dgieselaar mentioned this pull request Oct 11, 2021

[7.x] [APM] Display all properties by default (#113221) #114499

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 12, 2021

dgieselaar added a commit that referenced this pull request Oct 12, 2021

[7.x] [APM] Display all properties by default (#113221) (#114499)

deb016e

Co-authored-by: Kibana Machine <[email protected]>

This was referenced Nov 1, 2021

[APM] Enable access to all transaction and span attributes in metadata tab #67729

Closed

[APM] Show http.response.* in metadata view for RUM #55666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APM] Display all properties by default in flyout #113221

[APM] Display all properties by default in flyout #113221

dgieselaar commented Sep 28, 2021

elasticmachine commented Sep 28, 2021

dgieselaar commented Sep 28, 2021

formgeist commented Sep 28, 2021

dgieselaar commented Sep 28, 2021

formgeist commented Sep 28, 2021

mshustov Sep 28, 2021

dgieselaar Sep 28, 2021

dgieselaar Sep 30, 2021 •

edited

Loading

pgayvallet Sep 30, 2021

dgieselaar Sep 30, 2021

sorenlouv commented Sep 28, 2021

smith left a comment

smith Sep 29, 2021

dgieselaar Sep 30, 2021

smith Sep 29, 2021

dgieselaar Sep 30, 2021

smith Sep 29, 2021

dgieselaar Sep 30, 2021

smith commented Sep 29, 2021

sorenlouv commented Sep 29, 2021

sorenlouv Sep 29, 2021 •

edited

Loading

dgieselaar Sep 30, 2021

formgeist left a comment

formgeist Sep 30, 2021

dgieselaar Sep 30, 2021

dgieselaar commented Sep 30, 2021

mshustov left a comment

dgieselaar commented Oct 5, 2021

kibanamachine commented Oct 5, 2021

kibanamachine commented Oct 5, 2021

kibanamachine commented Oct 7, 2021

kibanamachine commented Oct 8, 2021

kibanamachine commented Oct 11, 2021

	import { SectionDescriptor } from './types';
	import type { SectionDescriptor } from './types';

@@ @@ -5,20 +5,49 @@ @@
                * 2.0.
                */
+              /*

	return get(response.body.hits.hits[0]._source, `${processorEvent}.id`);
	return response.body.hits.hits[0]._source[processorEvent].id;

[APM] Display all properties by default in flyout #113221

[APM] Display all properties by default in flyout #113221

Conversation

dgieselaar commented Sep 28, 2021

elasticmachine commented Sep 28, 2021

dgieselaar commented Sep 28, 2021

formgeist commented Sep 28, 2021

dgieselaar commented Sep 28, 2021

formgeist commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenlouv commented Sep 28, 2021

smith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smith commented Sep 29, 2021

sorenlouv commented Sep 29, 2021

sorenlouv Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formgeist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgieselaar commented Sep 30, 2021

mshustov left a comment

Choose a reason for hiding this comment

dgieselaar commented Oct 5, 2021

kibanamachine commented Oct 5, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

kibanamachine commented Oct 5, 2021

💔 Backport failed

kibanamachine commented Oct 7, 2021

kibanamachine commented Oct 8, 2021

kibanamachine commented Oct 11, 2021

dgieselaar Sep 30, 2021 •

edited

Loading

sorenlouv Sep 29, 2021 •

edited

Loading