Removed _Source use from `get_trace_items` #191647

bryce-b · 2024-08-28T14:54:24Z

Summary

This PR switched out _source for fields in the get_trace_items API, and updates all downstream dependencies of that data. Additional APIs changed: get_top_dependency_spans, get_derived_service_annotations, and trace_samples.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Are there performance risks associated with your feature? Does it potentially access or create: (1) many fields; (2) many indices; (3) lots of data; (4) lots of saved objects; (5) large saved objects
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

and fixed downstream impacts

obltmachine · 2024-08-28T14:54:50Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

felixbarny · 2024-09-03T14:51:37Z

x-pack/plugins/observability_solution/apm/common/waterfall/typings.ts

+  'transaction.type': string[];
+  'transaction.result'?: string[];
+  'faas.coldstart': boolean[];
+  'span.links'?: SpanLink[][];


We need to verify taking span links from fields works for our use case. My suspicion is that we need to take span links from _source because fields doesn't return an array of span link objects, but arrays for all leaf fields, such as span.links.trace.id. Also, the content for OTel span links and APM span links will look slightly differently.

Yes, it appears both will have to be specified: span.links.trace.id and span.links.span.id. Are there any additional leaf fields? I can experiment with collecting them and dedotting.

it seems like span.links.trace.id and span.links.trace.id are string types, while the processed structure is an array of SpanLink[] which implies there may be more than one span link per span, but I'm not able to find examples of spans with more than one SpanLink.

I did some tests and it appears that span.links.trace.id and span.links.span.id will be two equal length and the ids at relative indices are associated.

I was able to recreate the SpanLink[] structure by merging these two arrays together. A commit is coming shortly.

bryce-b · 2024-09-03T23:18:44Z

...ansaction_details/waterfall_with_summary/waterfall_container/waterfall_container.stories.tsx

@@ -71,21 +71,61 @@ export const Example: Story<any> = () => {
    return doc['processor.event'] === ProcessorEvent.error;
  });

-  const errorDocs = events.splice(errorEventId, 1);


The issue here is the format the synth-trace data is returned, specifically error.exception.message, which looks like:

error { exception: [ { message: "..." } ] }

And now that we are using fields, the expected format is flat: error.exception.message.
Is there a better way to do this? A 'fields' formatter for snyth-trace? A global 'flatten' method available, perhaps?

bryce-b · 2024-09-05T21:45:04Z

.../waterfall_with_summary/waterfall_container/waterfall/waterfall_helpers/waterfall_helpers.ts

+      dedot(
+        {
+          'trace.id': transaction[SPAN_LINKS_TRACE_ID]?.[i],
+          'span.id': transaction[SPAN_LINKS_SPAN_ID]?.[i],


This expression isn't ideal if these two arrays are not equal lengths. If they are not equal lengths, it means the map between the trace.ids and span.ids is wrong. I can add a ?? "0" to line 152 so it doesn't blow up, but is there a better way to handle this?

elasticmachine · 2024-09-05T21:52:46Z

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

bryce-b · 2024-09-05T23:32:28Z

x-pack/plugins/observability_solution/apm/common/waterfall/typings.ts

+  'parent.id'?: string[];
+  'error.id': string[];
+  'error.log.message'?: string[];
+  'error.exception'?: Exception[];


This needs to get resolved

It seems like the error.exception.stacktrace... fields are not accessible through the fields query. Should I try injecting them through the _sources response? @felixbarny

Hm, yes, I think the exception stack trace (and possibly span links) need to be fetched from _source. Make sure to only fetch the fields you need from source using source filtering.

Note that the structure will be different for OTel values. So we'll probably want to normalize/convert the data to have a consistent model the UI can work with.

crespocarlos

Hey Bryce, Is there anything we can do to the fields type, to make it behave like _sources on InferSearchResponseOf? I'm concerned about losing type safety. There are many places where we're forcing type casting, eg: fields[TRANSACTION_ID]?.[0] as string

If we manage do adjust the fields type we'll probably simplify the changes in this PR.

crespocarlos · 2024-09-06T09:11:49Z

x-pack/plugins/observability_solution/apm/common/waterfall/typings.ts

@@ -6,83 +6,61 @@
 */


Q: is there anything we can do to avoid changing the types here?

crespocarlos · 2024-09-06T09:13:26Z

x-pack/plugins/observability_solution/apm/common/waterfall/typings.ts

+  'timestamp.us': number[];
+  'trace.id': string[];
+  'service.name': string[];
+  'service.environment'?: string[];
+  'agent.name': string[];
+  'event.outcome'?: string[];
+  'parent.id'?: string[];
+  'processor.event': ['transaction'];
+  'transaction.duration.us': number[];
+  'transaction.id': string[];
+  'transaction.name': string[];
+  'transaction.type': string[];
+  'transaction.result'?: string[];
+  'faas.coldstart'?: boolean[];
+  'span.links.span.id'?: string[];
+  'span.links.trace.id'?: string[];


Q: Why is everything defined as arrays?

the data returned by 'fields' is always wrapped in an array : https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#search-fields-response

I tend to agree with @crespocarlos here. IMHO the APIs must return the same payload whether we query for _source or fields.

It shall be done.

crespocarlos · 2024-09-06T09:14:24Z

...ansaction_details/waterfall_with_summary/waterfall_container/waterfall_container.stories.tsx

+      Object.keys(event).forEach((key) => {
+        const item = event[key];
+        if (!Array.isArray(item)) {
+          event[key] = [item];
+        }
+      });


Tip
we could use reduce to create a new object out of an array

I'm not trying to create a new object from an array, I'm wrapping the values in the object with an array to match the new expected format that 'fields' returns. Maybe I'm misunderstanding your recommendation?

crespocarlos · 2024-09-06T09:14:37Z

...ansaction_details/waterfall_with_summary/waterfall_container/waterfall_container.stories.tsx

+    Object.keys(event).forEach((key) => {
+      const item = event[key];
+      if (!Array.isArray(item)) {
+        event[key] = [item];
+      }
+    });


Tip
we could use reduce to create a new object out of an array

kibana-ci · 2024-09-06T20:59:43Z

💔 Build Failed

Buildkite Build
Commit: 0fe5b64
Interpreting CI Failures
Storybooks Preview
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-191647-0fe5b64c4d0b

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #10 / APM API tests traces/find_traces.spec.ts basic no archive Find traces when traces exist when using EQL returns the correct trace samples for join sequences
[job] [logs] FTR Configs #10 / APM API tests traces/find_traces.spec.ts basic no archive Find traces when traces exist when using EQL returns the correct trace samples for join sequences
[job] [logs] Jest Tests #16 / Marker renders error marker
[job] [logs] Jest Tests #16 / Marker renders error marker

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1872	2083	+211

Async chunks

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

id	before	after	diff
`apm`	3.5MB	4.0MB	⚠️ +537.2KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`apm`	39.9KB	42.6KB	+2.7KB

History

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

cauemarcondes

Please make sure the APIs return the same payload as before. This will simplify the amount of work and all tests must keep working as expected.

bryce-b added 2 commits August 26, 2024 07:36

wip

111df68

removed _source usage from get_trace_items.ts

78e1b65

and fixed downstream impacts

bryce-b mentioned this pull request Aug 29, 2024

[APM] Avoid using _source for OTel compatibility #189947

Closed

felixbarny reviewed Sep 3, 2024

View reviewed changes

bryce-b added 3 commits September 3, 2024 16:16

removed comment

a2a0565

updated tests and fixes

fddcff4

fixed tests to reflect changes from removal of _source usage

18954f6

bryce-b commented Sep 3, 2024

View reviewed changes

changed span.links to use span.links.span.id and span.links.trace.id

3bd5c99

bryce-b commented Sep 5, 2024

View reviewed changes

bryce-b marked this pull request as ready for review September 5, 2024 21:51

bryce-b requested a review from a team as a code owner September 5, 2024 21:51

bryce-b added the release_note:enhancement label Sep 5, 2024

Merge branch 'main' into remove-_source-from-queries

6a4a8e8

botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Sep 5, 2024

bryce-b commented Sep 5, 2024

View reviewed changes

fixed additional fields missed

d438980

crespocarlos reviewed Sep 6, 2024

View reviewed changes

fixed EventOutcome type

96ad20e

bryce-b requested a review from felixbarny September 6, 2024 16:27

felixbarny requested a review from gregkalapos September 6, 2024 16:31

additional fixes

0fe5b64

cauemarcondes requested changes Sep 9, 2024

View reviewed changes

jennypavlova mentioned this pull request Sep 10, 2024

[Spacetime] OpenTelemetry data with Synthtrace #192465

Closed

bryce-b closed this Sep 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed _Source use from `get_trace_items` #191647

Removed _Source use from `get_trace_items` #191647

bryce-b commented Aug 28, 2024 •

edited

Loading

obltmachine commented Aug 28, 2024

felixbarny Sep 3, 2024

bryce-b Sep 4, 2024

bryce-b Sep 4, 2024 •

edited

Loading

bryce-b Sep 5, 2024

bryce-b Sep 3, 2024 •

edited

Loading

bryce-b Sep 5, 2024 •

edited

Loading

elasticmachine commented Sep 5, 2024

bryce-b Sep 5, 2024

bryce-b Sep 6, 2024

felixbarny Sep 9, 2024

crespocarlos left a comment

crespocarlos Sep 6, 2024

crespocarlos Sep 6, 2024

bryce-b Sep 6, 2024

cauemarcondes Sep 9, 2024

bryce-b Sep 9, 2024

crespocarlos Sep 6, 2024

bryce-b Sep 6, 2024

crespocarlos Sep 6, 2024

kibana-ci commented Sep 6, 2024 •

edited

Loading

cauemarcondes left a comment

Removed _Source use from get_trace_items #191647

Removed _Source use from get_trace_items #191647

Conversation

bryce-b commented Aug 28, 2024 • edited Loading

Summary

Checklist

Risk Matrix

For maintainers

obltmachine commented Aug 28, 2024

🤖 GitHub comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-b Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryce-b Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

bryce-b Sep 5, 2024 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crespocarlos 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 6, 2024 • edited Loading

💔 Build Failed

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

cauemarcondes left a comment

Choose a reason for hiding this comment

Removed _Source use from `get_trace_items` #191647

Removed _Source use from `get_trace_items` #191647

bryce-b commented Aug 28, 2024 •

edited

Loading

bryce-b Sep 4, 2024 •

edited

Loading

bryce-b Sep 3, 2024 •

edited

Loading

bryce-b Sep 5, 2024 •

edited

Loading

kibana-ci commented Sep 6, 2024 •

edited

Loading