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.

Sign up for GitHub

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][Otel] Errors: Add fallback to span id if the parent id is undefined #195796

Closed
wants to merge 1 commit into from
Closed

[APM][Otel] Errors: Add fallback to span id if the parent id is undefined #195796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions x-pack/plugins/observability_solution/apm/common/waterfall/typings.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export interface WaterfallTransaction {
coldstart?: boolean;
};
span?: {
id?: string;
links?: SpanLink[];
};
}
Expand Down Expand Up @@ -74,6 +75,7 @@ export interface WaterfallError {
trace?: { id: string };
transaction?: { id: string };
parent?: { id: string };
span?: { id?: string };
error: {
id: string;
log?: {
Expand Down
205 changes: 205 additions & 0 deletions ...ll_with_summary/waterfall_container/waterfall/waterfall_helpers/waterfall_helpers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,11 @@ import {
updateTraceTreeNode,
IWaterfallNode,
IWaterfallNodeFlatten,
getErrorItem,
} from './waterfall_helpers';
import { APMError } from '../../../../../../../../typings/es_schemas/ui/apm_error';
import {
WaterfallError,
WaterfallSpan,
WaterfallTransaction,
} from '../../../../../../../../common/waterfall/typings';
Expand Down Expand Up @@ -979,5 +981,208 @@ describe('waterfall_helpers', () => {
);
});
});

describe('getErrorItem', () => {
const items = [
{
docType: 'transaction',
doc: {
transaction: {
id: 'transactionEntryId',
name: 'transactionEntry',
},
span: {
id: 'spanId',
},
timestamp: {
us: 1728561267262487,
},
},
id: 'spanId',
duration: 11882,
},
{
docType: 'span',
doc: {
span: {
name: 'childOtel',
id: 'childId',
},
timestamp: {
us: 1728561267262497,
},
parent: {
id: 'spanId',
},
},
id: 'childId',
parentId: 'spanId',
parent: {
docType: 'transaction',
doc: {
transaction: {
id: 'spanId',
name: 'parent',
},
span: {
id: 'spanId',
},
timestamp: {
us: 1728561267262487,
},
trace: {
id: 'traceId',
},
},
id: 'spanId',
},
},
{
docType: 'span',
doc: {
span: {
name: 'childApm',
id: 'childId',
},
timestamp: {
us: 1728561267262497,
},
trace: {
id: 'traceId',
},
parent: {
id: 'spanId',
},
},
id: 'childId',
parentId: 'spanId',
parent: {
docType: 'transaction',
doc: {
transaction: {
id: 'spanId',
name: 'parent',
},
span: {
id: 'spanId',
},
timestamp: {
us: 1728561267262487,
},
trace: {
id: 'traceId',
},
},
id: 'spanId',
},
},
] as unknown as IWaterfallSpanOrTransaction[];
it('should return the parent if the parent id is present in the error (otel + apm server case)', () => {
const error = {
error: {
grouping_key: '14f4d08792a45fce53a46c93851e36e1',
exception: [
{
type: '*errors.errorString',
handled: true,
message: 'boom',
},
],
id: 'af2a24b8d8fed8bf6d027f24117fc729',
},
timestamp: {
us: 1728561267273422,
},
trace: {
id: 'traceId',
},
service: {
name: 'sendotlp-otel-apm-server',
},
processor: {
event: 'error',
},
parent: {
id: 'childId',
},
} as WaterfallError;

expect(getErrorItem(error, items)).toMatchObject({
parentId: 'childId',
docType: 'error',
doc: error,
});
});
it('should return the parent if the span id is present in the error (otel native data case)', () => {
const error = {
error: {
grouping_key: '14f4d08792a45fce53a46c93851e36e1',
exception: [
{
type: '*errors.errorString',
handled: true,
message: 'boom',
},
],
id: '312895daf95d975e2fd2d6c6c2f9d2d5',
},
timestamp: {
us: 1728561267273422,
},
trace: {
id: 'traceId',
},
service: {
name: 'sendotlp-otel-collector',
},
processor: {
event: 'error',
},
span: {
id: 'childId',
},
} as WaterfallError;

expect(getErrorItem(error, items)).toMatchObject({
parentId: 'childId',
docType: 'error',
doc: error,
});
});
it('should return the parent/parentId as undefined if the span id/parent id is not present in the error', () => {
const error = {
error: {
grouping_key: '14f4d08792a45fce53a46c93851e36e1',
exception: [
{
type: '*errors.errorString',
handled: true,
message: 'boom',
},
],
id: '312895daf95d975e2fd2d6c6c2f9d2d5',
},
timestamp: {
us: 1728561267273422,
},
trace: {
id: 'traceId',
},
service: {
name: 'sendotlp-no-parent',
},
processor: {
event: 'error',
},
} as WaterfallError;

expect(getErrorItem(error, items)).toMatchObject({
parentId: undefined,
parent: undefined,
docType: 'error',
doc: error,
});
});
});
});
});
8 changes: 4 additions & 4 deletions ...terfall_with_summary/waterfall_container/waterfall/waterfall_helpers/waterfall_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,15 +161,15 @@ function getSpanItem(span: WaterfallSpan, linkedChildrenCount: number = 0): IWat
};
}

function getErrorItem(
export function getErrorItem(
error: WaterfallError,
items: IWaterfallItem[],
entryWaterfallTransaction?: IWaterfallTransaction
): IWaterfallError {
const entryTimestamp = entryWaterfallTransaction?.doc.timestamp.us ?? 0;
const parent = items.find((waterfallItem) => waterfallItem.id === error.parent?.id) as
| IWaterfallSpanOrTransaction
| undefined;
const parent = items.find(
(waterfallItem) => waterfallItem.id === (error.parent?.id ?? error.span?.id)
Copy link
Contributor

Choose a reason for hiding this comment

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

@jennypavlova have you considered moving this logic to the server instead of handling it on the client? I ask that because we'll probably have many hidden problems if we support multiple schemas on the client. As @AlexanderWert has already pointed out there's another place where the same logic is needed.

I wonder if the UI should be schema-agnostic and respect its own schema.

This is similar to a draft PR @rmyz opened recently #194100 (review).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the comment, @cauemarcondes
In general, I agree with you but this is just a fallback to a different field if the parent id is not there, I wouldn't call it a 'new' schema. It can be confusing if we modify the response and return the parent.id in a document that doesn't have it but it probably helps us in the long run as we will have the parent id available in all places in the UI. I remember that we discussed with @dgieselaar and @gregkalapos that we wanted to do a fallback on the client to keep it compatible and return the fields in the same format as they come from elasticsearch in the server.
But we can do it on the server, just need the changes made in #195242 as a base and merge a PR there

@AlexanderWert has already #195796 (comment) there's another place where the same logic is needed.

This is unrelated. I investigated more and this happens because the transaction.id is missing and the query and it is not returning the transaction object - even if I do what you suggested it won't fix the issue mentioned there

image

It's a different query (changed here) I am not sure why we are missing the transaction id in the error document - I see that also span id is not there (using the test env)

Copy link
Member

Choose a reason for hiding this comment

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

@jennypavlova ahhh right, great finding!

I think it's not that easy (maybe impossible) to actually have the transaction.id on OTel errors, because it cannot be enriched from other attributes but would be required to be collected by the SDKs / APM Agents. That's a conceptual limitation. So, then I think let's proceed with this PR as is.

\cc @gregkalapos @felixbarny correct me if I'm wrong with the above

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 used span id for Otel errors - the PR is merged to #195242

) as IWaterfallSpanOrTransaction | undefined;

const errorItem: IWaterfallError = {
docType: 'error',
Expand Down
Loading