Skip to content

[Discover][APM] Unify Span/Transaction into single Overview tab#233716

Merged
Bluefinger merged 19 commits intoelastic:mainfrom
Bluefinger:span-transaction-overview-merge
Sep 5, 2025
Merged

[Discover][APM] Unify Span/Transaction into single Overview tab#233716
Bluefinger merged 19 commits intoelastic:mainfrom
Bluefinger:span-transaction-overview-merge

Conversation

@Bluefinger
Copy link
Contributor

@Bluefinger Bluefinger commented Sep 2, 2025 •
edited
Loading

Summary

This PR is for cleaning up technical debt regarding the split of Span/Transaction concepts for the Overview tab, and unifying them into a single Overview that can handle the particularities of each while deduplicating code for everything else that overlaps.

Closes #232603

image

TODO

  • Initial merge and resolving conflicts from main
  • Migrate remaining/missing test cases to Overview
  • Further testing to ensure no regressions

How to Test

  • Ensure the space is set to Observability mode, and then go to Discover. Ensure a traces index is being used in either Classic mode or ES|QL mode.
  • In Classic mode, selecting a trace document should show the same visual behaviour for both transactions and spans as before. Same with ES|QL mode.
  • In ES|QL mode, modify the query to have at least trace.id and a few other fields. The overview should show widgets for fields it can resolve.
  • in ES|QL mode, modify the query to remove trace.id from the kept fields. The overview tab should no longer show (as the minimum we expect for a trace document is to have the trace.id present).

@Bluefinger Bluefinger added Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. Project:OneDiscover Enrich Discover with contextual awareness backport:version Backport to applied version labels v9.2.0 labels Sep 2, 2025
@Bluefinger
Copy link
Contributor Author

Bluefinger commented Sep 2, 2025

/ci

@Bluefinger Bluefinger marked this pull request as ready for review September 3, 2025 09:56
@Bluefinger Bluefinger requested a review from a team September 3, 2025 09:56
@Bluefinger Bluefinger requested review from a team as code owners September 3, 2025 09:56
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 3, 2025

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

@Bluefinger Bluefinger added the release_note:skip Skip the PR/issue when compiling release notes label Sep 3, 2025
@iblancof iblancof self-requested a review September 3, 2025 11:43
cauemarcondes
cauemarcondes reviewed Sep 3, 2025
View reviewed changes
...nents/observability/traces/components/full_screen_waterfall/span_flyout/span_flyout_body.tsx
@@ -58,7 +56,6 @@ export interface SpanFlyoutProps {

export const SpanFlyoutBody = ({ hit, loading, dataView }: SpanFlyoutProps) => {
Copy link
Contributor

@cauemarcondes cauemarcondes Sep 3, 2025

Choose a reason for hiding this comment

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

Does it make sense to rename this component too?

Suggested change
export const SpanFlyoutBody = ({ hit, loading, dataView }: SpanFlyoutProps) => {
export const TraceFlyoutBody = ({ hit, loading, dataView }: SpanFlyoutProps) => {

Copy link
Contributor Author

@Bluefinger Bluefinger Sep 3, 2025

Choose a reason for hiding this comment

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

At the moment this depends on span.id right? Maybe this should stay as SpanFlyout for now until this has support for using trace.id as well? So more for the follow-up task #232608? What do you think?

iblancof
iblancof reviewed Sep 3, 2025
View reviewed changes
Copy link
Contributor

@iblancof iblancof left a comment

Choose a reason for hiding this comment

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

Tested locally and left a few comments!
Can’t wait to get this merged and integrate the changes into my current work 🚀

.../components/observability/traces/doc_viewer_overview/sub_components/dependency_name_link.tsx
spanType?: string;
spanSubtype?: string;
environment: string;
environment?: string;
Copy link
Contributor

@iblancof iblancof Sep 3, 2025

Choose a reason for hiding this comment

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

Why is this optional now? Is it related to OTel purposes?

iblancof
iblancof approved these changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@iblancof iblancof left a comment

Choose a reason for hiding this comment

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

Leaving my approval to unblock things!
However, I’d like us to align on something regarding the Tab title.

@iblancof iblancof mentioned this pull request Sep 3, 2025
Merged
crespocarlos
crespocarlos approved these changes Sep 4, 2025
View reviewed changes
Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

Cool stuff. I didn't test it, just reviewed the code.

@Bluefinger Bluefinger enabled auto-merge (squash) September 4, 2025 08:50
@Bluefinger Bluefinger disabled auto-merge September 4, 2025 08:55
achyutjhunjhunwala
achyutjhunjhunwala approved these changes Sep 4, 2025
View reviewed changes
Copy link
Contributor

@achyutjhunjhunwala achyutjhunjhunwala left a comment

Choose a reason for hiding this comment

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

Reviewed obs-ux-logs changes alone. They are LGTM

davismcphee
davismcphee approved these changes Sep 4, 2025
View reviewed changes
Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code-only review, Data Discovery changes LGTM, although I have concerns about the changes to the document profile heuristics.

...wareness/profile_providers/observability/traces_document_profile/document_profile/profile.ts
import { createGetDocViewer } from './accessors';

const OBSERVABILITY_TRACES_SPAN_DOCUMENT_PROFILE_ID = 'observability-traces-span-document-profile';
const OBSERVABILITY_TRACES_SPAN_DOCUMENT_PROFILE_ID = 'observability-traces-document-profile';
Copy link
Contributor

@davismcphee davismcphee Sep 4, 2025

Choose a reason for hiding this comment

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

Just an fyi in case your team is currently tracking telemetry in Discover that this will have an impact on it.

...wareness/profile_providers/observability/traces_document_profile/document_profile/profile.ts

return dataStreamType === 'traces' && (isApmSpan || isOtelSpan);
};
return dataSourceContext.category === DataSourceCategory.Traces && !!traceId;
Copy link
Contributor

@davismcphee davismcphee Sep 4, 2025

Choose a reason for hiding this comment

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

This new dataSourceContext.category check means the traces document profile will no longer work in mixed data scenarios, e.g. FROM metrics-*,traces-*, since it depends on the data source profile being active too. Ultimately it's up to your team, but it takes away from the One Discover goal and purpose of document profiles, and I wouldn't recommend it.

Copy link
Contributor Author

@Bluefinger Bluefinger Sep 4, 2025

Choose a reason for hiding this comment

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

I tested with FROM remote_cluster:logs-*, remote_cluster:traces-*, and I was still able to get the Trace Overview tab for trace documents and the Log overview tab for logs. This check is done per document, so I don't think this will cause problems?

Copy link
Contributor Author

@Bluefinger Bluefinger Sep 4, 2025

Choose a reason for hiding this comment

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

After a bit of discussion, it seems there is an issue with how data source profiles are resolved and are currently clobbering each other, so this will be solved in a separate task: #234068

@Bluefinger Bluefinger enabled auto-merge (squash) September 4, 2025 16:21
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 5, 2025

💔 Build Failed

Failed CI Steps

History

@Bluefinger Bluefinger merged commit 58fcb3d into elastic:main Sep 5, 2025
13 checks passed
shahargl pushed a commit to shahargl/kibana that referenced this pull request Sep 7, 2025
@Bluefinger @shahargl
[Discover][APM] Unify Span/Transaction into single Overview tab (elas…
27b9585 
…tic#233716)

## Summary

This PR is for cleaning up technical debt regarding the split of
Span/Transaction concepts for the Overview tab, and unifying them into a
single Overview that can handle the particularities of each while
deduplicating code for everything else that overlaps.

Closes elastic#232603

<img width="486" height="1097" alt="image"
src="https://github.com/user-attachments/assets/c2ba3eeb-eddb-47ae-8326-1ea23a19703b"
/>


## TODO

- [x] Initial merge and resolving conflicts from main
- [x] Migrate remaining/missing test cases to Overview
- [x] Further testing to ensure no regressions

## How to Test

- Ensure the space is set to Observability mode, and then go to
Discover. Ensure a traces index is being used in either Classic mode or
ES|QL mode.
- In Classic mode, selecting a trace document should show the same
visual behaviour for both transactions and spans as before. Same with
ES|QL mode.
- In ES|QL mode, modify the query to have at least `trace.id` and a few
other fields. The overview should show widgets for fields it can
resolve.
- in ES|QL mode, modify the query to remove `trace.id` from the kept
fields. The overview tab should no longer show (as the minimum we expect
for a trace document is to have the `trace.id` present).
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 9, 2025
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 9, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

3 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 10, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 11, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 12, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

KodeRad pushed a commit to KodeRad/kibana that referenced this pull request Sep 15, 2025
@Bluefinger @KodeRad
[Discover][APM] Unify Span/Transaction into single Overview tab (elas…
6085d83 
…tic#233716)

## Summary

This PR is for cleaning up technical debt regarding the split of
Span/Transaction concepts for the Overview tab, and unifying them into a
single Overview that can handle the particularities of each while
deduplicating code for everything else that overlaps.

Closes elastic#232603

<img width="486" height="1097" alt="image"
src="https://github.com/user-attachments/assets/c2ba3eeb-eddb-47ae-8326-1ea23a19703b"
/>


## TODO

- [x] Initial merge and resolving conflicts from main
- [x] Migrate remaining/missing test cases to Overview
- [x] Further testing to ensure no regressions

## How to Test

- Ensure the space is set to Observability mode, and then go to
Discover. Ensure a traces index is being used in either Classic mode or
ES|QL mode.
- In Classic mode, selecting a trace document should show the same
visual behaviour for both transactions and spans as before. Same with
ES|QL mode.
- In ES|QL mode, modify the query to have at least `trace.id` and a few
other fields. The overview should show widgets for fields it can
resolve.
- in ES|QL mode, modify the query to remove `trace.id` from the kept
fields. The overview tab should no longer show (as the minimum we expect
for a trace document is to have the `trace.id` present).
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 15, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

3 similar comments
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 16, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 17, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 18, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 233716 locally
cc: @Bluefinger

@Bluefinger Bluefinger added backport:skip This PR does not require backporting and removed backport:version Backport to applied version labels labels Sep 19, 2025
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 19, 2025
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Sep 24, 2025
@Bluefinger @CAWilson94
[Discover][APM] Unify Span/Transaction into single Overview tab (elas…
ebc4c44 
…tic#233716)

## Summary

This PR is for cleaning up technical debt regarding the split of
Span/Transaction concepts for the Overview tab, and unifying them into a
single Overview that can handle the particularities of each while
deduplicating code for everything else that overlaps.

Closes elastic#232603

<img width="486" height="1097" alt="image"
src="https://github.com/user-attachments/assets/c2ba3eeb-eddb-47ae-8326-1ea23a19703b"
/>


## TODO

- [x] Initial merge and resolving conflicts from main
- [x] Migrate remaining/missing test cases to Overview
- [x] Further testing to ensure no regressions

## How to Test

- Ensure the space is set to Observability mode, and then go to
Discover. Ensure a traces index is being used in either Classic mode or
ES|QL mode.
- In Classic mode, selecting a trace document should show the same
visual behaviour for both transactions and spans as before. Same with
ES|QL mode.
- In ES|QL mode, modify the query to have at least `trace.id` and a few
other fields. The overview should show widgets for fields it can
resolve.
- in ES|QL mode, modify the query to remove `trace.id` from the kept
fields. The overview tab should no longer show (as the minimum we expect
for a trace document is to have the `trace.id` present).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cauemarcondes cauemarcondes cauemarcondes left review comments

@iblancof iblancof iblancof approved these changes

@crespocarlos crespocarlos crespocarlos approved these changes

@achyutjhunjhunwala achyutjhunjhunwala achyutjhunjhunwala approved these changes

@davismcphee davismcphee davismcphee approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting Project:OneDiscover Enrich Discover with contextual awareness release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. v9.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Discover][APM] Deduplicate logic between Span/Transaction Overviews into a single Overview tab

8 participants

@Bluefinger @elasticmachine @kibanamachine @crespocarlos @achyutjhunjhunwala @davismcphee @cauemarcondes @iblancof