[APM][OTel] EDOT error summary fix #217885

jennypavlova · 2025-04-10T16:35:58Z

Summary

This PR fixes the issue with the error summary missing items using edot. It includes e2e tests with synthtrace for both edot and otel services.

TODO

Test with serverless (waiting for the PR to be deployed)
Tested on serverless works as expected:

gregkalapos · 2025-04-11T06:11:29Z

.../plugins/apm/public/components/app/error_group_details/error_sampler/error_sample_detail.tsx

-  const userAgent = error?.user_agent;
+  const errorUrl =
+    (error.error.page?.url || error.url?.full) ?? buildUrl(error?.url ? error : transaction);
+  const method = error.http?.request?.method ?? transaction?.http?.request?.method;


This is just a super nit thing, and maybe not even relevant, nevertheless:

Since we want to make our product OTel native and transaction is not an OTel concept and we have countless issues by having it, this caught my eye. I know this is already existing code, and we need backwards compatibility, where transaction is a real thing.

Nevertheless: transaction here is just a variable name, that point to the root span, right? Any thought on maybe going with rootSpan or something like that?

This is probably all good, it just worries me a bit, that transaction keeps spreading in the code.

This is really a nit and unfortunately I don't really have a suggestion here - this is more like an fyi for consideration.

Hey @gregkalapos,

Nevertheless: transaction here is just a variable name, that point to the root span, right? Any thought on maybe going with rootSpan or something like that?

I checked the logic to get the "transaction" data from the error and we use transaction id OR span id (ref) so some of those fallbacks (also the existing ones before my change) were used for the APM use-cases (in case some of the information is not present in the error we were checking the associated transaction)

Since we want to make our product OTel native and transaction is not an OTel concept and we have countless issues by having it, this caught my eye. I know this is already existing code, and we need backwards compatibility, where transaction is a real thing.

I totally agree that we should go with the OTel native approach but also the whole "error" concept is different when we compare APM and OTel. As we still use this one for APM transactions or OTel spans I would keep the transaction name for now (otherwise I nee to change many different places that are not part of the errors logic I am fixing here). As this might create confusion for OTel I will add a comment above to explain and will be happy to migrate to OTel schema everywhere (which would require more changes but would make it much easier to work with when we have OTel data) but anyway thanks for the comment :)

elasticmachine · 2025-04-11T14:29:31Z

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

github-actions · 2025-04-11T14:30:16Z

🤖 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!)

github-actions · 2025-04-11T16:20:13Z

🤖 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!)

jennypavlova · 2025-04-11T16:21:54Z

/oblt-deploy

rmyz · 2025-04-14T09:02:48Z

.../plugins/apm/public/components/app/error_group_details/error_sampler/error_sample_detail.tsx

+    transaction?.url?.full ||
+    buildUrl(error?.url ? error : transaction);
+  const method = error.http?.request?.method ?? transaction?.http?.request?.method;
+  const status = error.http?.response?.status_code ?? transaction?.http?.response?.status_code;


Can we create a variable to know if we are actually using the error or the transaction object?
I think it will be hard to debug as it is right now, since we don't know which value are we using with that many || and ??.
Something like const errorData = error.X ? error : transaction; and use it then const method = objectKind.http?.request?.method.... The naming can be improved but is just an example, wdyt?

Thanks for the comment, I understand it looks a bit confusing. I tried to extract some parts to some new variables. You can take a look at the latest changes and let me know if you think that I should change them :)

Thanks for applying the changes @jennypavlova !
I was thinking about checking where the data is once and then using that variable, for example:
const source = error.http ? error : transaction.
Then we can use const userAgent = source?.user_agent.
The objective would be to remove redundant checks, as we will have all the information in one place, whether it's error or transaction, does it make sense?

Hey @rmyz, I tried to do something similar at the beginning but there is no guarantee that if the field X is present in an error all the other fields will be as well (I couldn't find such field - I know in theory this should not happen but for example the user agent fallback was added even before this otel changes here.

const source = error.http ? error : transaction. Then we can use const userAgent = source?.user_agent.

So the problem I see here is that we could have the error.http but the user_agent to come from a transaction. I kept the other check to be safe and to make sure we always have a fallback if the error values are not there

rmyz

Code LGTM, added a small nit

crespocarlos · 2025-04-14T09:03:38Z

x-pack/solutions/observability/plugins/apm/public/utils/build_url.ts

+  };
+  server?: {
+    address?: string;
+    port?: string;


Should port be a string?

https://opentelemetry.io/docs/specs/semconv/attributes-registry/server/#server-port
https://www.elastic.co/guide/en/apm/server/current/exported-fields-ecs.html

Do you mean just string and not optional? It is string now 🤔

I guess the confusion is because the Opentelemetry port is considered an Integer, while we treat it as a string

it's an integer on both ECS and semconv

Hmm true, same in ECS, I guess I got it mixed with a different property... I will set it to be a number everywhere, thank you both!

I pushed some other changes to make sure we convert it to number where needed and set the number type everywhere

crespocarlos · 2025-04-14T09:04:15Z

.../plugins/apm/public/components/app/error_group_details/error_sampler/error_sample_detail.tsx

                <EuiIcon type="discoverApp" />
              </EuiFlexItem>
-              <EuiFlexItem style={{ whiteSpace: 'nowrap' }}>
+              <EuiFlexItem css={{ whiteSpace: 'nowrap' }}>


rmyz

LGTM

crespocarlos

LGTM!

jennypavlova · 2025-04-15T17:58:22Z

Check serverless with latest changes (EDOT and APM cases) ✅

elasticmachine · 2025-04-15T19:00:43Z

💚 Build Succeeded

Buildkite Build
Commit: 7e7474f
Cloud Deployment
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-217885-7e7474fc7593

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`	2.6MB	2.6MB	+437.0B

History

cc @jennypavlova

kibanamachine · 2025-04-15T19:44:29Z

Starting backport for target branches: 8.x, 9.0

https://github.com/elastic/kibana/actions/runs/14478043479

kibanamachine · 2025-04-15T19:50:41Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts You might need to backport the following PRs to 8.x: - [ES\|QL] fix editor menus on safari (#218167) - [Custom threshold rule] Allow group for ip type fields !! (#216062) - [Security Solution][Alert flyout] Edit highlighted fields in overview tab (#216740) - [Attack Discovery][Scheduling] Fix the attack discovery alert type (#218025) - [ES\|QL] remove worker (#218006) - [Solution Side Nav] Remove PanelContentProvider & support optional title in nav node (#218156) - [Synthetics] Sync global parameters is called in the endpoints to add, edit or delete global params (#216197) - 🌊 Streams: Use better default field (#217478) - [Security Solution][Detection Engine] Split search request building from search (#216887) - fix: [ML] Data Frame Analytics: Analytics selection flyout missing title from announcement (#217666)
❌	9.0	Backport failed because of merge conflicts You might need to backport the following PRs to 9.0: - [Synthetics] Sync global parameters is called in the endpoints to add, edit or delete global params (#216197) - Fix/lens serialize (#218082) - Auto increase fields limit of the alert indices (#216719) - [Console] Fix output for empty response body (#218104)

Manual backport

To create the backport manually run:

node scripts/backport --pr 217885

Questions ?

Please refer to the Backport tool documentation

## Summary This PR fixes the issue with the error summary missing items using edot. It includes e2e tests with synthtrace for both edot and otel services. TODO - [x] Test with serverless (waiting for the PR to be deployed) Tested on serverless works as expected: <img width="2560" alt="image" src="https://github.com/user-attachments/assets/8dd7962e-7d66-482d-97fb-0b08882bd04f" /> (cherry picked from commit 7c9a3ee) # Conflicts: # x-pack/solutions/observability/plugins/apm/ftr_e2e/cypress/e2e/service_overview/otel_edot_service_overview_and_transactions.cy.ts # x-pack/solutions/observability/plugins/apm/ftr_e2e/cypress/fixtures/synthtrace/adservice_edot.ts

jennypavlova · 2025-04-16T08:43:03Z

💚 All backports created successfully

Status	Branch	Result
✅	9.0
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

## Summary This PR fixes the issue with the error summary missing items using edot. It includes e2e tests with synthtrace for both edot and otel services. TODO - [x] Test with serverless (waiting for the PR to be deployed) Tested on serverless works as expected: <img width="2560" alt="image" src="https://github.com/user-attachments/assets/8dd7962e-7d66-482d-97fb-0b08882bd04f" /> (cherry picked from commit 7c9a3ee) # Conflicts: # x-pack/solutions/observability/plugins/apm/ftr_e2e/cypress/e2e/service_overview/otel_edot_service_overview_and_transactions.cy.ts # x-pack/solutions/observability/plugins/apm/ftr_e2e/cypress/fixtures/synthtrace/adservice_edot.ts

# Backport This will backport the following commits from `main` to `8.x`: - [[APM][OTel] EDOT error summary fix (#217885)](#217885)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

# Backport This will backport the following commits from `main` to `9.0`: - [[APM][OTel] EDOT error summary fix (#217885)](#217885)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport)

[APM][OTel] Edot error summary fix

679abca

gregkalapos reviewed Apr 11, 2025

View reviewed changes

jennypavlova added 3 commits April 11, 2025 14:33

Add comment explaining transaction / root span logic

c6c2f7a

Add error summary tests

e05eea6

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

2e61e8e

jennypavlova self-assigned this Apr 11, 2025

jennypavlova added release_note:fix v9.0.0 Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Apr 11, 2025

jennypavlova marked this pull request as ready for review April 11, 2025 14:29

jennypavlova requested a review from a team as a code owner April 11, 2025 14:29

jennypavlova added the ci:project-deploy-observability Create an Observability project label Apr 11, 2025

jennypavlova added ci:cloud-redeploy Always create a new Cloud deployment ci:project-deploy-observability Create an Observability project and removed ci:project-deploy-observability Create an Observability project labels Apr 11, 2025

jennypavlova added the ci:build-serverless-image label Apr 11, 2025

jennypavlova added 4 commits April 11, 2025 19:16

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

f365f99

Fix fallback

a73cb33

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

94c5671

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

cfdc899

rmyz reviewed Apr 14, 2025

View reviewed changes

crespocarlos reviewed Apr 14, 2025

View reviewed changes

jennypavlova force-pushed the fix-edot-error-summary-transaction-fallback branch from 7471321 to f3314ab Compare April 14, 2025 09:28

jennypavlova requested review from crespocarlos and rmyz April 14, 2025 15:29

Cleanup

3b854b7

jennypavlova force-pushed the fix-edot-error-summary-transaction-fallback branch from f3314ab to 3b854b7 Compare April 14, 2025 15:41

jennypavlova added 5 commits April 14, 2025 17:42

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

bb3bb7b

Fix server port type

3fabe85

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

caaf16f

Convert port type

6393ee1

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

99b6c5b

rmyz approved these changes Apr 15, 2025

View reviewed changes

jennypavlova added 2 commits April 15, 2025 16:04

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

5b3b636

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

1bca9c2

crespocarlos approved these changes Apr 15, 2025

View reviewed changes

Merge branch 'main' into fix-edot-error-summary-transaction-fallback

7e7474f

jennypavlova enabled auto-merge (squash) April 15, 2025 17:58

jennypavlova merged commit 7c9a3ee into elastic:main Apr 15, 2025
10 checks passed

jennypavlova mentioned this pull request Apr 16, 2025

[9.0] [APM][OTel] EDOT error summary fix (#217885) #218390

Merged

jennypavlova mentioned this pull request Apr 16, 2025

[8.x] [APM][OTel] EDOT error summary fix (#217885) #218391

Merged

kibanamachine mentioned this pull request Apr 16, 2025

[APM][OTel] Encode service name in the APM URLs #217092

Merged

kibanamachine added the v9.0.1 label Apr 17, 2025

[APM][OTel] EDOT error summary fix #217885

[APM][OTel] EDOT error summary fix #217885

Uh oh!

Conversation

jennypavlova commented Apr 10, 2025 • edited by kibanamachine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Apr 11, 2025

Uh oh!

github-actions bot commented Apr 11, 2025

🤖 GitHub comments

Uh oh!

github-actions bot commented Apr 11, 2025

🤖 GitHub comments

Uh oh!

jennypavlova commented Apr 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmyz left a comment

Choose a reason for hiding this comment

Uh oh!

crespocarlos left a comment

Choose a reason for hiding this comment

Uh oh!

jennypavlova commented Apr 15, 2025

Uh oh!

elasticmachine commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

Uh oh!

kibanamachine commented Apr 15, 2025

Uh oh!

kibanamachine commented Apr 15, 2025

💔 All backports failed

Manual backport

Questions ?

Uh oh!

jennypavlova commented Apr 16, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jennypavlova commented Apr 10, 2025 •

edited by kibanamachine

Loading

elasticmachine commented Apr 15, 2025 •

edited

Loading