[Traces][Discover] Prevent flyout remount when switching document types in Trace Waterfall#250406
Conversation
|
/ci |
f8d8580 to
b45524d
Compare
|
/ci |
| } | ||
|
|
||
| export function useSpanFlyoutData({ spanId, traceId }: UseSpanFlyoutDataParams): SpanFlyoutData { | ||
| const { span, loading } = useFetchSpan({ spanId, traceId }); |
There was a problem hiding this comment.
Note for reviewer:
Code reference:
|
|
||
| const title = i18n.translate( | ||
| 'unifiedDocViewer.observability.traces.fullScreenWaterfall.spanFlyout.title', | ||
| { |
There was a problem hiding this comment.
Note for reviewer:
Code reference:
| <WaterfallFlyout | ||
| flyoutId={spanFlyoutId} | ||
| onCloseFlyout={onCloseFlyout} | ||
| <Overview |
There was a problem hiding this comment.
Note for reviewer:
Code reference:
| } | ||
|
|
||
| export function useLogFlyoutData({ id, index }: UseLogFlyoutDataParams): LogFlyoutData { | ||
| const { loading, log, index: resolvedIndex } = useFetchLog({ id, index }); |
There was a problem hiding this comment.
note for reviewer:
Code reference:
| }, [id, log, resolvedIndex]); | ||
|
|
||
| const title = i18n.translate( | ||
| 'unifiedDocViewer.observability.traces.fullScreenWaterfall.logFlyout.title.log', |
There was a problem hiding this comment.
| /> | ||
| ) : null} | ||
| </WaterfallFlyout> | ||
| <LogsOverview hit={hit} dataView={logDataView} indexes={indexes} showTraceWaterfall={false} /> |
There was a problem hiding this comment.
|
/ci |
|
/oblt-deploy |
|
Pinging @elastic/obs-exploration-team (Team:obs-exploration) |
|
/oblt-deploy |
81aed43 to
e7ec995
Compare
|
/oblt-deploy |
iblancof
left a comment
There was a problem hiding this comment.
Tested locally and it feels way better this way 🚀
| }: DocumentDetailFlyoutProps) { | ||
| const isSpanType = type === spanFlyoutId; | ||
|
|
||
| const spanData = useSpanFlyoutData({ |
There was a problem hiding this comment.
Would it make sense to keep the hooks inside SpanFlyoutContent/LogFlyoutContent, so we avoid calling both here when only one is actually needed?
There was a problem hiding this comment.
good question!! Is part of the strategy, I'll add a comment just in case but, both hooks short-circuit with empty strings, so no unnecessary API calls are made. We keep them here because WaterfallFlyout needs some data, for example the loading to render the loading skeleton properly
There was a problem hiding this comment.
I’m still unsure about the global architecture here.
For example:
DocumentDetailFlyout expects type because it needs to pass it to WaterfallFlyout, which also expects a type to use as flyoutId (is it really needed like this?). Meanwhile, FlyoutContent is also expecting type, but it’s receiving both spanData and logData anyway. It then decides its content based on type for spans and on logData for logs. It’s a bit confusing, as we’re mixing the source of truth for some of the logic here.
Wdyt about giving it another look?
There was a problem hiding this comment.
Good catch! I’ll go part by part so I can explain it and you can check if I’m missing something.
expects a type to use as flyoutId (is it really needed like this?)
id: You’re right, it's done
It then decides its content based on type for spans and on logData for logs. It’s a bit confusing
You're right, it was inconsistent. I've made it consistent now, both cases check type explicitly.
Global architecture
I understand the confusion here. FlyoutContent acts as a wrapper to avoid something like this:
<WaterfallFlyout
flyoutId={type}
onCloseFlyout={onCloseFlyout}
dataView={dataView}
hit={hit}
loading={loading}
title={title}
>
{hit && isSpanType ? (
<SpanFlyoutContent hit={hit} dataView={dataView} activeSection={activeSection} />
) : null}
{hit && !isSpanType && logData.logDataView ? (
<LogFlyoutContent
hit={hit}
logDataView={logData.logDataView}
error={logData.error}
/>
) : null}
</WaterfallFlyout>imho, that approach is more confusing. With FlyoutContent, we wrap the rendering logic in a more controlled way.
Summary up the overall approach:
Why both hooks are called + FlyoutContent exists:
- DocumentDetailFlyout acts as a unified entry point to prevent flyout unmount/remount when switching document types (e.g., clicking an error badge from a span to view a log). That's why we need a "God" component.
- Both hooks are called (with short-circuit for the inactive one) because we need hit, loading, and title for WaterfallFlyout regardless of type.
- FlyoutContent encapsulates the rendering decision in one place, we can do inline as well is just a thing of criteria.
Besides that, I created the useDocumentFlyoutData hook to consolidate the component’s business logic in one place. Let me know if it’s easier to understand this way. I’m also open to other suggestions if you see a cleaner approach with the same result.
...traces/components/full_screen_waterfall/waterfall_flyout/span_flyout/use_span_flyout_data.ts
Show resolved
Hide resolved
...observability/traces/components/full_screen_waterfall/waterfall_flyout/logs_flyout/index.tsx
Outdated
Show resolved
Hide resolved
...vability/traces/components/full_screen_waterfall/waterfall_flyout/document_detail_flyout.tsx
Outdated
Show resolved
Hide resolved
...traces/components/full_screen_waterfall/waterfall_flyout/span_flyout/use_span_flyout_data.ts
Show resolved
Hide resolved
f4e5c8d to
8575e1e
Compare
.../traces/components/full_screen_waterfall/waterfall_flyout/logs_flyout/use_log_flyout_data.ts
Outdated
Show resolved
Hide resolved
...traces/components/full_screen_waterfall/waterfall_flyout/span_flyout/use_span_flyout_data.ts
Outdated
Show resolved
Hide resolved
8575e1e to
51c5c3c
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
|
…es in Trace Waterfall (elastic#250406) Closes: elastic#249695 ## Summary Previously, the trace waterfall had two separate flyout components for spans and logs. While this design was architecturally valid, it caused a suboptimal user experience: when switching between document types (e.g., clicking an error badge to view a log), the flyout would unmount and remount, causing a visual flash and unnecessary loading states. ## Solution: Unified DocumentDetailFlyout We introduced `DocumentDetailFlyout` as a single entry point that internally delegates to the appropriate content component based on the document type. Note: These components are designed specifically for the trace waterfall context and are not intended for reuse elsewhere. If standalone document flyouts are needed in the future, a more generalized abstraction can be considered at that time. ``` ┌─────────────────────────────────────────────────────────────┐ │ FullScreenWaterfall │ │ (manages docId, docIndex, activeFlyoutType, activeSection) │ └─────────────────────────┬───────────────────────────────────┘ │ ▼ ┌─────────────────────────────────────────────────────────────┐ │ DocumentDetailFlyout │ │ - Discriminates by `type` (spanFlyoutId | logsFlyoutId) │ │ - Calls appropriate data hook │ │ - Renders WaterfallFlyout wrapper │ └─────────────────────────┬───────────────────────────────────┘ │ ┌───────────────┴───────────────┐ ▼ ▼ ┌─────────────────────┐ ┌─────────────────────┐ │ SpanFlyoutContent │ │ LogFlyoutContent │ └─────────────────────┘ └─────────────────────┘ ``` ## Key Architectural Decisions ### 1. Hook + Content Separation Each flyout type is split into: - **Data hook** (`useSpanFlyoutData`, `useLogFlyoutData`) - fetches and transforms data - **Content component** (`SpanFlyoutContent`, `LogFlyoutContent`) - renders the Overview tab **Why?** - Allows `DocumentDetailFlyout` to call both hooks and select the appropriate data - Content components receive only what they need (no null checks internally) - Easier to test data fetching separately from rendering ## Usage from Parent The parent component (`FullScreenWaterfall`) only needs to: 1. Track the document ID and active flyout type 2. Optionally track the document index (for logs) and active section (for span scroll-to behavior) 3. Pass the trace dataView for the Table/JSON tabs The unified flyout handles all the complexity of fetching data and rendering the appropriate content. ## Demo https://github.com/user-attachments/assets/c0d8124d-b006-4199-9cfb-933284fb1ffa ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [ ] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [ ] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. ### Identify risks Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss. Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging. - [ ] [See some risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) - [ ] ...
…iew_cps * commit '32efd9b2fb078ade51073fd2d0068bc74c029d6b': (49 commits) [Security Solution] Rules exceptions subfeatures (elastic#245722) [BK] Upgrade axios (elastic#251150) Fix AI Connector form fields resetting to default value when cleared by user (elastic#251095) deduplicate otel dependencies (elastic#250841) Adds initial agents.md file (elastic#250833) [index management] Faster index list loading (elastic#246276) skip failing test suite (elastic#251086) skip failing test suite (elastic#251048) [Security Solutions] Trial Companion - adjust UX design (elastic#250910) [Traces][Discover] Prevent flyout remount when switching document types in Trace Waterfall (elastic#250406) [DOCS][Cases][9.4 & Serverless]: Doc new `Maximum amount of cases to open` setting for case action (elastic#250993) [Discover][Traces] Explore trace.id from logs in Discover (elastic#249632) Remove ! from SOs docs link (elastic#251097) [ML] Maps: Add telemetry events for file uploads (elastic#247543) [Fleet] Fix dupplicate ids when copying an integration policy or an agent policy (elastic#250971) [Dashboards as Code] Add snake case object keys util (elastic#250962) [Core] Remove URL Overflow & Deprecate `storeInSessionStorage` setting (elastic#242972) [One Workflow] fix: Fix Variable Retrieval in Workflow Execution Engine (elastic#250852) Rework Elastic Managed LLMs page (elastic#251069) [Lens powered by ES|QL] Update Switch to Query mode modal warning message (elastic#251051) ...
Closes: #249695
Summary
Previously, the trace waterfall had two separate flyout components for spans and logs. While this design was architecturally valid, it caused a suboptimal user experience: when switching between document types (e.g., clicking an error badge to view a log), the flyout would unmount and remount, causing a visual flash and unnecessary loading states.
Solution: Unified DocumentDetailFlyout
We introduced
DocumentDetailFlyoutas a single entry point that internally delegates to the appropriate content component based on the document type.Note: These components are designed specifically for the trace waterfall context and are not intended for reuse elsewhere. If standalone document flyouts are needed in the future, a more generalized abstraction can be considered at that time.
Key Architectural Decisions
1. Hook + Content Separation
Each flyout type is split into:
useSpanFlyoutData,useLogFlyoutData) - fetches and transforms dataSpanFlyoutContent,LogFlyoutContent) - renders the Overview tabWhy?
DocumentDetailFlyoutto call both hooks and select the appropriate dataUsage from Parent
The parent component (
FullScreenWaterfall) only needs to:The unified flyout handles all the complexity of fetching data and rendering the appropriate content.
Demo
demo.mov
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.