Discover:Traces Red Metrics #11030
Discover:Traces Red Metrics #11030
Conversation
Signed-off-by: Adam Tackett <[email protected]>
WalkthroughAdds RED (Request, Error, Latency) metrics for Discover:Traces: new trace query builders, parallel trace query thunks and orchestration, a processor converting aggregation results to chart-ready data, UI and styling for three trace metric panels with field-missing error handling, tests, and persistence defaults adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Explore UI
participant Store as Redux Store
participant Orch as Query Orchestrator
participant Search as Search API
participant Processor as TraceAggregationProcessor
participant Charts as Chart Components
UI->>Store: dispatch executeQueries (Traces)
Store->>Orch: prepareTraceCacheKeys & start three trace thunks
Orch->>Search: executeRequestCountQuery
Orch->>Search: executeErrorCountQuery
Orch->>Search: executeLatencyQuery
Search-->>Store: request result
Search-->>Store: error result
Search-->>Store: latency result
Store->>Processor: processTraceAggregationResults(request,error,latency,dataset,interval,timeField,dataPlugin,rawInterval,uiSettings)
Processor-->>Store: emit requestChartData, errorChartData, latencyChartData, bucketInterval
Store->>UI: update results state
UI->>Charts: render three metric panels (or show field-missing / error callouts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Adam Tackett <[email protected]>
ℹ️ Manual Changeset Creation ReminderPlease ensure manual commit for changeset file 11030.yml under folder changelogs/fragments to complete this PR. If you want to use the available OpenSearch Changeset Bot App to avoid manual creation of changeset file you can install it in your forked repository following this link. For more information about formatting of changeset files, please visit OpenSearch Auto Changeset and Release Notes Tool. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11030 +/- ##
========================================
Coverage 60.78% 60.78%
========================================
Files 4538 4540 +2
Lines 122603 122874 +271
Branches 20597 20678 +81
========================================
+ Hits 74522 74694 +172
- Misses 42805 42871 +66
- Partials 5276 5309 +33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
TackAdam
requested review from
AMoo-Miki,
BionIT,
Flyingliuhub,
SuZhou-Joe,
ZilongX,
abbyhu2000,
ananzh,
ashwin-pc,
bandinib-amzn,
curq,
joshuarrrr,
kavilla,
manasvinibs,
ruanyl,
xinruiba,
zengyan-amazon,
zhongnansu and
zhyuanqi
as code owners
TackAdam
requested review from
LDrago27,
Maosaic,
angle943,
d-buckner,
huyaboo,
joshuali925,
mengweieric,
paulstn,
ps48,
raintygao,
ruchidh,
sejli,
sumukhswamy,
virajsanghvi and
wanglam
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (22)
src/plugins/explore/public/components/chart/_histogram.scss (2)
23-25: Use SCSS variables for consistency with the project's design system.The new
&Contentselector introduces a flex layout adjustment, which is appropriate. However, this aligns with the broader observation about the new trace chart styles not utilizing the project's SCSS variable system.
67-102: Consider refactoring hard-coded dimensions to use SCSS variables for consistency.The new
.exploreTracesChartblock introduces comprehensive styling for the trace metrics display. However, the styling uses hard-coded pixel values (200px, 32px, 14px, 14px, 600) throughout, while the existing codebase extensively uses SCSS variables (e.g.,$euiSize,$euiFontWeightRegular).Additionally, there's potential redundancy: both
&__container(withmin-height: 200px; max-height: 200px;) and&__section(withheight: 200px;) explicitly constrain height to the same value. Consider whether both constraints are intentional or if this can be simplified.To align with project conventions, consider extracting these values to variables or using the EUI size/spacing system. For example:
// At the top of the file or in a variables file $traces-chart-height: 200px; $traces-spacer-width: 32px; .exploreTracesChart { &__container { min-height: $traces-chart-height; max-height: $traces-chart-height; } &__section { height: $traces-chart-height; } // ... rest of the selectors }Alternatively, if these dimensions are part of the EUI design system, use existing EUI tokens (e.g.,
$euiSize * 25or similar) instead of hard-coded values.src/plugins/explore/public/application/utils/interfaces.ts (1)
140-147: Consider using a more specific type foruiSettings.Using
anytype reduces type safety. If there's an existing type for UI settings (e.g.,IUiSettingsClientfrom core), consider using it instead.+import { IUiSettingsClient } from '../../../../../core/public'; + export type HistogramDataProcessor = ( rawResults: ISearchResult, dataset: Dataset, data: DataPublicPluginStart, interval: string, - uiSettings: any, + uiSettings: IUiSettingsClient, breakdownField?: string ) => ProcessedSearchResults;src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
44-74: Consider adding a type for the date histogram aggregation config.The
as anytype cast on line 46 reduces type safety. While the implementation is functional, consider defining an interface or using the appropriate type from the aggregation framework to improve maintainability.The property override pattern using
deletefollowed byObject.definePropertyis unconventional but works. A comment explaining why this approach is necessary (presumably because the originalbucketsproperty is defined as a getter on the prototype or instance) would help future maintainers understand this pattern.src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (5)
15-20: Consider typingbucketIntervalmore strictly.Using
anyforbucketIntervalloses type safety. Consider using theBucketIntervaltype from interfaces.ts or a more specific type definition.+import { BucketInterval } from '../../../interfaces'; + export interface TraceAggregationResults { requestChartData: ChartData; errorChartData: ChartData; latencyChartData: ChartData; - bucketInterval: any; + bucketInterval: BucketInterval; }
99-101: Silent catch blocks may hide debugging issues.Both catch blocks silently swallow errors. Consider adding at least a debug-level log or a comment explaining why errors are intentionally ignored. This aids debugging in production scenarios.
} catch (error) { - // Fall back to hardcoded format if histogram config fails + // Fall back to hardcoded format if histogram config fails + // eslint-disable-next-line no-console + console.debug('Histogram config failed, using fallback format:', error); }Also applies to: 141-143
235-246: Unused variabletotalCount.
totalCountis computed but never used. Either remove it or document its intended future purpose.const dataMap = new Map<number, number>(); - let totalCount = 0; const actualIntervalMs = extractPPLIntervalMs(results, intervalMs); for (const hit of results.hits.hits) { // Look for span() field or direct time field const spanTimeKey = Object.keys(hit._source || {}).find((key) => key.startsWith('span(')); const timeValue = spanTimeKey ? hit._source?.[spanTimeKey] : hit._source?.[timeField]; const requestCount = hit._source?.['request_count'] || hit._source?.['count()'] || 0; - totalCount += requestCount; - if (timeValue && !isNaN(requestCount)) {
260-264: Consider adding a safeguard against excessive bucket generation.If the time range is large and interval is small, the bucket-filling loop could generate an excessive number of buckets, potentially impacting performance or causing memory issues. Consider adding a maximum bucket count check.
const startBucket = Math.floor(minTime / actualIntervalMs) * actualIntervalMs; const endBucket = Math.floor(maxTime / actualIntervalMs) * actualIntervalMs; + const maxBuckets = 10000; // Reasonable upper bound + const estimatedBuckets = Math.ceil((endBucket - startBucket) / actualIntervalMs); + if (estimatedBuckets > maxBuckets) { + // Return sparse data from dataMap only to avoid memory issues + const sortedKeys = Array.from(dataMap.keys()).sort((a, b) => a - b); + for (const bucketKey of sortedKeys) { + chartValues.push({ x: bucketKey, y: dataMap.get(bucketKey) || 0 }); + xAxisOrderedValues.push(bucketKey); + } + return { /* ... chart structure */ }; + } + for (let bucketKey = startBucket; bucketKey <= endBucket; bucketKey += actualIntervalMs) {
221-282: Consider extracting common logic from the three process functions.
processRequestCountData,processErrorCountData, andprocessLatencyDatashare significant code duplication (~80% identical). Consider refactoring into a single generic function with configuration for field names and labels.This could be addressed in a follow-up refactor:
interface ChartProcessConfig { metricField: string | string[]; yAxisLabel: string; transformer?: (value: number) => number; } function processChartData( results: ISearchResult, config: ChartProcessConfig, // ... common params ): ChartData { /* ... */ }Also applies to: 284-341, 343-402
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (1)
325-350: Consider verifying console behavior in error handling test.The
consoleSpyis created but not verified. Consider either asserting that console.error was/wasn't called, or removing the spy if it's not needed.it('should handle processing errors gracefully', () => { const consoleSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); // Create results that will cause Date parsing errors const errorResults = createMockSearchResult([ { 'span(endTime,5m)': 'definitely-not-a-date', 'count()': 'not-a-number', }, ]); const result = processTraceAggregationResults( errorResults, null, null, mockDataset, '5m', 'endTime' ); // Should still return a result structure expect(result).toBeDefined(); expect(result.requestChartData).toBeDefined(); + // Verify no unhandled errors were logged (or document expected behavior) + // expect(consoleSpy).not.toHaveBeenCalled(); consoleSpy.mockRestore(); });src/plugins/explore/public/components/chart/discover_chart_container.tsx (2)
43-43: Unused selectordateRange.
dateRangeis selected from state but not used in this component. Remove it if not needed.- const dateRange = useSelector((state: RootState) => state.queryEditor.dateRange);
74-93: Fragile partial state construction forcreateHistogramConfigWithInterval.Creating a partial
RootStatewithas RootStatecast is fragile. IfcreateHistogramConfigWithIntervalis modified to access additional state properties, this will cause runtime issues. Consider:
- Passing only the required values directly to avoid the state dependency
- Or using the actual Redux selector with
useSelectorconst actualInterval = useMemo(() => { if (flavorId === ExploreFlavor.Traces && dataset && services?.data && interval) { - // Create a minimal getState function for createHistogramConfigWithInterval - const getState = () => - ({ - legacy: { interval }, - queryEditor: { breakdownField }, - } as RootState); - - const histogramConfig = createHistogramConfigWithInterval( - dataset, - interval, - services, - getState, - TRACES_CHART_BAR_TARGET - ); + // Use createHistogramConfigs directly instead of going through the wrapper + const histogramConfigs = createHistogramConfigs( + dataset, + interval, + services.data, + services.uiSettings, + undefined, + TRACES_CHART_BAR_TARGET + ); + // Extract interval from configs if available + const finalInterval = histogramConfigs?.aggs?.[1]?.buckets?.getInterval()?.expression; - return histogramConfig?.finalInterval || interval || 'auto'; + return finalInterval || interval || 'auto'; } return interval || 'auto'; }, [flavorId, dataset, services, interval, breakdownField]);src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx (1)
48-52: Query action mocks appear unused.The mocks for
executeRequestCountQuery,executeErrorCountQuery, andexecuteLatencyQueryare defined but don't appear to be used in any test assertions. If these are needed to prevent actual dispatch calls, this is fine; otherwise, consider removing them.src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (2)
6-12: Consider usingfromDateandtoDatefields or removing them.The
TraceAggregationConfiginterface includesfromDateandtoDatefields, but they are never used in any of the query builder functions. If time range filtering is handled elsewhere (e.g., viatimefilter), consider removing these unused fields to avoid confusion. Otherwise, incorporate them into the queries.
14-48: DRY violation:buildTraceAggregationQueriesduplicates individual query builders.The logic in
buildTraceAggregationQueriesis identical to the individualbuildRequestCountQuery,buildErrorCountQuery, andbuildLatencyQueryfunctions. Consider refactoring to use the individual functions:export const buildTraceAggregationQueries = (baseQuery: string, config: TraceAggregationConfig) => { - const { timeField, interval, breakdownField } = config; - - const requestCountQuery = breakdownField - ? `${baseQuery} | stats count() by span(${timeField}, ${interval}), ${breakdownField} | sort ${timeField}` - : `${baseQuery} | stats count() by span(${timeField}, ${interval}) | sort ${timeField}`; - - // Error Count Query - Count error spans per time bucket - const errorCondition = `status.code=2`; - const errorCountQuery = breakdownField - ? `${baseQuery} | where ${errorCondition} | stats count() as error_count by span(${timeField}, ${interval}), ${breakdownField} | sort ${timeField}` - : `${baseQuery} | where ${errorCondition} | stats count() as error_count by span(${timeField}, ${interval}) | sort ${timeField}`; - - // Latency Query - Average duration per time bucket - const latencyQuery = breakdownField - ? `${baseQuery} | where isnotnull(durationInNanos) | stats avg(durationInNanos) as avg_duration_nanos by span(${timeField}, ${interval}), ${breakdownField} | eval avg_latency_ms = avg_duration_nanos / 1000000 | sort ${timeField}` - : `${baseQuery} | where isnotnull(durationInNanos) | stats avg(durationInNanos) as avg_duration_nanos by span(${timeField}, ${interval}) | eval avg_latency_ms = avg_duration_nanos / 1000000 | sort ${timeField}`; - return { - requestCountQuery, - errorCountQuery, - latencyQuery, + requestCountQuery: buildRequestCountQuery(baseQuery, config), + errorCountQuery: buildErrorCountQuery(baseQuery, config), + latencyQuery: buildLatencyQuery(baseQuery, config), }; };src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (3)
200-201: Consider using a more specific type foruiSettings.The
uiSettingsparameter is typed asany. For better type safety, consider usingIUiSettingsClientfrom the core public types.+import { IUiSettingsClient } from 'opensearch-dashboards/public'; + export const histogramResultsProcessor: HistogramDataProcessor = ( rawResults: ISearchResult, dataset: DataView, data: DataPublicPluginStart, interval: string, - uiSettings: any + uiSettings: IUiSettingsClient ): ProcessedSearchResults => {
296-329: Simplify thetimeFieldNamefallback on line 325.Since
dataset.timeFieldNameis already validated to be truthy on line 303, the fallback|| 'endTime'on line 325 is unreachable. Consider removing it to avoid confusion:const config = createTraceAggregationConfig( - dataset.timeFieldName || 'endTime', + dataset.timeFieldName, calculatedInterval, fromDate, toDate );
810-852:executeTraceAggregationQueriesduplicates cache key generation.The cache keys are hardcoded as inline template strings (e.g.,
`trace-requests:${baseQuery}`), duplicating the logic inprepareTraceCacheKeys. Consider reusingprepareTraceCacheKeysto ensure consistency:Note: This would require passing the
Queryobject instead of justbaseQuery, or extracting the cache key prefix logic into a shared constant.src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (1)
190-198: Test helper includes fields not inTraceAggregationConfig.The
createTraceConfighelper includesdurationFieldandstatusFieldproperties that are not part of theTraceAggregationConfiginterface defined intrace_aggregation_builder.ts. These fields appear to be test-only and may cause confusion. Consider aligning the helper with the actual interface or documenting the test-specific extensions.const createTraceConfig = (overrides: any = {}) => ({ timeField: 'endTime', interval: '5m', fromDate: 'now-1h', toDate: 'now', - durationField: 'durationInNanos', - statusField: 'status.code', ...overrides, });src/plugins/explore/public/components/chart/explore_traces_chart.tsx (3)
26-30: Move import statement before the constant declaration.The import statement on line 30 appears after the
tracesIntervalOptionsconstant declaration. While this is valid JavaScript, it's unconventional and may cause confusion. Consider moving all imports to the top of the file.import { DataPublicPluginStart, search } from '../../../../data/public'; +import { TimechartHeader, TimechartHeaderBucketInterval } from './timechart_header'; +import { DiscoverHistogram } from './histogram/histogram'; const tracesIntervalOptions = search.aggs.intervalOptions.filter((option) => { // Only exclude milliseconds (ms) - PPL doesn't support it, all others work return option.val !== 'ms'; }); -import { TimechartHeader, TimechartHeaderBucketInterval } from './timechart_header'; -import { DiscoverHistogram } from './histogram/histogram';
110-139: Consider extracting the error type to reduce duplication.The same error object shape is repeated three times for
requestError,errorQueryError, andlatencyError. Consider extracting a shared type:interface ChartQueryError { statusCode: number; error: string; message: { details: string; reason: string; type?: string; }; originalErrorMessage: string; } interface ExploreTracesChartProps { // ... requestError?: ChartQueryError | null; errorQueryError?: ChartQueryError | null; latencyError?: ChartQueryError | null; // ... }
325-371: Fallback error message could be i18n-ized.The fallback error message on line 363 (
'Failed to load request count data...') is a hardcoded string. For consistency with the other messages that usei18n.translate, consider wrapping this in an i18n call as well.<p> {requestError.originalErrorMessage || requestError.message?.details || requestError.error || - 'Failed to load request count data. Please try again or check your query.'} + i18n.translate('explore.traces.error.requestChartFallback', { + defaultMessage: 'Failed to load request count data. Please try again or check your query.', + })} </p>The same applies to the error and latency chart fallback messages on lines 431 and 493.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
changelogs/fragments/11030.yml(1 hunks)src/plugins/data/public/index.ts(2 hunks)src/plugins/explore/public/application/utils/interfaces.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_chart_data_processor.test.ts(0 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_chart_data_processor.ts(0 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts(15 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts(5 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/utils.ts(2 hunks)src/plugins/explore/public/application/utils/state_management/constants.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts(3 hunks)src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts(2 hunks)src/plugins/explore/public/components/chart/_histogram.scss(2 hunks)src/plugins/explore/public/components/chart/discover_chart_container.test.tsx(9 hunks)src/plugins/explore/public/components/chart/discover_chart_container.tsx(4 hunks)src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx(1 hunks)src/plugins/explore/public/components/chart/explore_traces_chart.tsx(7 hunks)src/plugins/explore/public/components/chart/timechart_header/timechart_header.test.tsx(1 hunks)src/plugins/explore/public/components/chart/timechart_header/timechart_header.tsx(3 hunks)src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts(2 hunks)src/plugins/explore/public/components/container/bottom_container/bottom_right_container/bottom_right_container.test.tsx(2 hunks)src/plugins/explore/public/components/container/bottom_container/bottom_right_container/bottom_right_container.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/plugins/explore/public/application/utils/state_management/actions/processors/trace_chart_data_processor.ts
- src/plugins/explore/public/application/utils/state_management/actions/processors/trace_chart_data_processor.test.ts
🧰 Additional context used
🧬 Code graph analysis (13)
src/plugins/explore/public/components/chart/timechart_header/timechart_header.test.tsx (1)
src/plugins/explore/public/components/chart/timechart_header/timechart_header.tsx (1)
TimechartHeader(100-221)
src/plugins/explore/public/application/utils/state_management/actions/utils.ts (1)
src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
createHistogramConfigs(16-82)
src/plugins/explore/public/components/container/bottom_container/bottom_right_container/bottom_right_container.tsx (1)
src/plugins/explore/public/components/chart/discover_chart_container.tsx (1)
DiscoverChartContainer(33-209)
src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (6)
TraceAggregationConfig(6-12)createTraceAggregationConfig(67-81)buildRequestCountQuery(39-48)buildErrorCountQuery(50-57)buildLatencyQuery(59-65)buildTraceAggregationQueries(14-37)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (3)
src/plugins/explore/public/application/utils/interfaces.ts (1)
ChartData(43-52)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
defaultResultsProcessor(110-141)src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
createHistogramConfigs(16-82)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (2)
extractPPLIntervalMs(187-219)processTraceAggregationResults(22-146)
src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
src/plugins/data/public/index.ts (3)
search(476-504)UI_SETTINGS(290-290)UI_SETTINGS(350-350)
src/plugins/explore/public/components/chart/explore_traces_chart.tsx (3)
src/plugins/explore/public/components/chart/timechart_header/timechart_header.tsx (1)
TimechartHeaderBucketInterval(40-44)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (2)
defaultPrepareQueryString(76-85)prepareHistogramCacheKey(90-94)src/plugins/explore/public/components/chart/histogram/histogram.tsx (1)
DiscoverHistogram(133-425)
src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx (1)
src/plugins/explore/public/components/chart/explore_traces_chart.tsx (1)
ExploreTracesChart(147-509)
src/plugins/explore/public/components/chart/discover_chart_container.test.tsx (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
processTraceAggregationResults(22-146)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (4)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (7)
histogramResultsProcessor(196-231)defaultPrepareQueryString(76-85)prepareTraceCacheKeys(96-103)executeTraceAggregationQueries(810-852)executeRequestCountQuery(854-876)executeErrorCountQuery(878-900)executeLatencyQuery(902-924)src/plugins/home/public/application/sample_data_client.test.js (1)
mockServices(38-44)src/plugins/data/public/index.ts (1)
Query(535-535)src/core/public/application/scoped_history.ts (1)
action(120-123)
src/plugins/explore/public/components/chart/discover_chart_container.tsx (5)
src/plugins/explore/public/application/utils/state_management/actions/utils.ts (1)
createHistogramConfigWithInterval(242-295)src/plugins/explore/public/application/utils/state_management/constants.ts (1)
TRACES_CHART_BAR_TARGET(14-14)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
prepareTraceCacheKeys(96-103)src/plugins/explore/public/application/utils/interfaces.ts (2)
ProcessedSearchResults(114-117)TracesChartProcessedResults(122-127)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
processTraceAggregationResults(22-146)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (4)
src/plugins/explore/public/helpers/get_flavor_from_app_id.ts (1)
getCurrentFlavor(29-34)src/plugins/explore/public/application/utils/state_management/actions/utils.ts (1)
createHistogramConfigWithInterval(242-295)src/plugins/explore/public/application/utils/state_management/constants.ts (1)
TRACES_CHART_BAR_TARGET(14-14)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (5)
createTraceAggregationConfig(67-81)TraceAggregationConfig(6-12)buildRequestCountQuery(39-48)buildErrorCountQuery(50-57)buildLatencyQuery(59-65)
🪛 Biome (2.1.2)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
[error] 7-7: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts
[error] 11-11: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (44)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: bundle-analyzer
🔇 Additional comments (37)
src/plugins/data/public/index.ts (1)
472-498: LGTM - Public API extension for time bucket utilities.The new exports (
calcAutoIntervalNearandTimeBuckets) are appropriately added to thesearch.aggsnamespace, making them available for the trace aggregation and histogram configuration logic in the explore plugin.src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.ts (3)
105-124: LGTM - Proper handling of legacy state columns during URL state loading.The logic correctly handles the following scenarios:
- Falls back to defaults when legacy state or columns are missing/empty
- Applies column corrections based on flavor when needed
- Preserves original columns when no correction is required
469-482: Verify the intended behavior for non-standard columns in logs flavor.In logs flavor (line 473-476), columns are only replaced when
!hasLogsColumns && hasTracesColumns. This means if a user has custom columns (e.g.,['customField1', 'customField2']) that don't include_sourceor traces columns, they will be preserved.If this is intentional (preserving user customizations), the logic is correct. If non-standard columns should also be replaced with defaults, the condition should be simplified.
433-488: LGTM - Well-structured column validation helper.The
getColumnsForDatasetfunction provides good separation of concerns:
- Graceful error handling returning
nullon failures- Clear logic flow for determining when to replace columns
- Proper async handling for dataView lookups
src/plugins/explore/public/application/utils/state_management/utils/redux_persistence.test.ts (2)
586-693: LGTM - Comprehensive test coverage for empty columns handling.The tests effectively cover:
- Empty columns array falling back to logs defaults
- Empty columns falling back to traces defaults for traces flavor
- Preservation of existing valid columns from URL state
880-1081: LGTM - Thorough test coverage for flavor switching column validation.The test suite covers important scenarios:
- Replacing logs columns with traces defaults when switching to traces flavor
- Replacing traces columns with logs defaults when switching to logs flavor
- Preserving valid columns when they match the current flavor
- Handling dataset type mismatches with current flavor
This provides good regression protection for the column validation logic.
changelogs/fragments/11030.yml (1)
1-2: LGTM - Changelog entry properly formatted.The changelog fragment correctly documents the new RED metrics charts feature with a proper PR reference.
src/plugins/explore/public/components/chart/timechart_header/timechart_header.test.tsx (1)
162-170: LGTM!The test correctly verifies the
hideIntervalSelectorprop behavior. It follows the existing test patterns in the file and properly validates that the interval selector is hidden when the prop is set totrue.src/plugins/explore/public/application/utils/state_management/constants.ts (1)
9-14: LGTM!The constant is well-documented and the value of 20 buckets for three Traces charts (vs. 50 for a single Logs chart) is a reasonable trade-off to maintain chart usability.
src/plugins/explore/public/components/container/bottom_container/bottom_right_container/bottom_right_container.tsx (1)
96-96: LGTM!Removing the flavor-based conditional allows the chart container to render for both Logs and Traces. The
DiscoverChartContainercomponent internally handles flavor differentiation, rendering appropriate chart types based onflavorId.src/plugins/explore/public/components/container/bottom_container/bottom_right_container/bottom_right_container.test.tsx (2)
283-295: LGTM!Test correctly updated to expect the chart container to render for Traces flavor, aligning with the production code change that enables trace chart rendering.
297-309: LGTM!Test name updated for clarity, and assertion correctly verifies chart container renders for Logs flavor.
src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.test.ts (1)
1-159: Comprehensive test suite for trace aggregation builders.The tests thoroughly cover:
- Configuration creation with and without breakdown fields
- All three query builders (request count, error count, latency)
- Combined query building
- Interval handling across various time units
- Complex base queries with filters
Good coverage of the PPL query construction logic.
src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (2)
6-14: LGTM!Clean extraction of
TimeBucketsfromsearch.aggsand proper imports of UI settings dependencies.
16-22: Breaking change: function signature updated with requireduiSettingsparameter.The insertion of
uiSettingsas a required parameter before the optionalbreakdownFieldchanges the call signature. Ensure all existing callers have been updated to pass theuiSettingsclient instance (the IUiSettingsClient from CoreStart/CoreSetup).src/plugins/explore/public/components/chart/timechart_header/timechart_header.tsx (1)
94-98: LGTM! Clean implementation of the optionalhideIntervalSelectorprop.The new prop is properly typed as optional with a sensible default value of
false, ensuring backward compatibility. The conditional rendering correctly gates the interval selector UI.Also applies to: 111-111, 144-213
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (1)
1-512: Comprehensive test coverage for the trace aggregation processor.The test suite provides excellent coverage including:
- Interval extraction from PPL span fields
- Processing of all three chart types (request, error, latency)
- Combined processing scenarios
- Error handling for malformed data, invalid timestamps, and missing fields
- Edge cases like large time ranges and null/undefined values
src/plugins/explore/public/components/chart/discover_chart_container.tsx (1)
116-151: LGTM! Clean separation of processing logic for Traces vs Logs.The processing logic correctly branches based on
flavorId, using the appropriate processor for each flavor. The dependency array is comprehensive.src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx (1)
68-378: Comprehensive test coverage for trace chart error handling.The test suite effectively covers:
- Individual field missing errors (durationInNanos, status, endTime, @timestamp)
- Multiple simultaneous field missing errors
- Non-field-missing error handling (generic server errors)
- Normal rendering when no errors present
The assertions correctly verify both the error messages and the rendering state of unaffected charts.
src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (2)
50-57: Hardcodedstatus.code=2limits compatibility to data-prepper schema.The error condition
status.code=2is hardcoded, which matches the PR description stating this is "currently only compatible with the data-prepper schema." Consider making this configurable inTraceAggregationConfigif supporting other schemas is planned.As noted in the PR objectives, this is intentionally limited to data-prepper. Ensure this limitation is documented and that a follow-up task exists for supporting additional schemas if needed.
67-81: LGTM!The factory function provides a clean API for creating trace aggregation configs.
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (3)
48-57: LGTM!The imports are well-organized, bringing in the necessary trace aggregation utilities and flavor detection helpers.
96-103: LGTM!The
prepareTraceCacheKeysfunction provides a clean API for generating consistent cache keys for the three RED metric queries.
854-924: LGTM!The three trace query thunks (
executeRequestCountQuery,executeErrorCountQuery,executeLatencyQuery) follow a consistent pattern and correctly delegate toexecuteQueryBase.src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (4)
152-175: LGTM!The mocks for
trace_aggregation_builderandgetCurrentFlavorare well-structured and provide the necessary test doubles for the trace aggregation workflow.
2028-2104: LGTM!The
executeTraceAggregationQueriestests thoroughly verify parallel execution, error handling, and cache key usage.
2170-2282: LGTM!The
executeRequestCountQuerytests provide good coverage for success, error, and no-results scenarios, including specific field-missing error handling.
2523-2609: LGTM!The integration tests for trace queries cover complex scenarios including query abortion and various time field configurations.
src/plugins/explore/public/components/chart/explore_traces_chart.tsx (4)
37-45: Unused import:clearResults.The import
clearResultsis imported but only used intimefilterUpdateHandler. However,onChangeIntervalusesclearResultsByKeyfor selective cache clearing. Verify ifclearResultsis still needed or if it should be replaced with selective clearing intimefilterUpdateHandlerfor consistency.
53-103: LGTM!The error handling utilities provide clear, user-friendly messages for missing field scenarios. The dynamic time field handling and fallback to generic messages are well-designed.
173-205: LGTM!The
onChangeIntervalhandler properly clears all chart-related cache keys and resets their query statuses toUNINITIALIZEDbefore re-executing queries. This ensures a clean state for the new interval.
308-505: LGTM!The three-chart layout with proper error states, loading spinners, and accessibility labels is well-implemented. Each chart conditionally renders based on data availability and error state.
src/plugins/explore/public/components/chart/discover_chart_container.test.tsx (5)
95-118: LGTM!The mocks for trace-related query functions are properly structured, providing the necessary test doubles for the trace aggregation workflow.
121-132: LGTM!The mock for
processTraceAggregationResultscorrectly returns the expected shape withrequestChartData,errorChartData, andlatencyChartData.
134-144: LGTM!The mock for
createHistogramConfigWithIntervalprovides a sensible default configuration for tests.
215-238: LGTM!The trace-related cache keys are properly added to the preloaded results state, enabling tests to exercise trace data processing paths.
313-328: LGTM!The test correctly uses
processTraceAggregationResultsinstead of the oldtracesHistogramResultsProcessor, aligning with the updated trace aggregation workflow.
src/plugins/explore/public/application/utils/state_management/actions/utils.ts
Show resolved
Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (4)
7-7: Shadowing of globalDataViewproperty.The
DataViewimport shadows the browser's built-inDataView(typed array view). While this is unlikely to cause issues in practice since the globalDataViewisn't used here, consider using an alias for clarity.-import { DataView } from '../../../../../../../data/common'; +import { DataView as DataViewType } from '../../../../../../../data/common';Then update all usages of
DataViewtoDataViewTypein this file.
99-101: Silent error suppression hides potential issues.Empty catch blocks make debugging difficult. Consider logging errors at a minimum, or propagating contextual information about what failed.
} catch (error) { - // Fall back to hardcoded format if histogram config fails + // Fall back to hardcoded format if histogram config fails + // eslint-disable-next-line no-console + console.debug('Failed to create histogram config, using fallback format:', error); }
141-143: Silent error suppression during processing.The outer catch block silently swallows all processing errors. This could mask serious issues. At minimum, log the error for debugging purposes.
} catch (error) { - // Error during processing + // Error during processing - log for debugging but return partial results + // eslint-disable-next-line no-console + console.error('Error processing trace aggregation results:', error); }
233-246: Unused variabletotalCount.The
totalCountvariable is computed on line 246 but never used. Either remove it or use it for validation/logging.if (results.hits?.hits) { const dataMap = new Map<number, number>(); - let totalCount = 0; const actualIntervalMs = extractPPLIntervalMs(results, intervalMs); for (const hit of results.hits.hits) { // Look for span() field or direct time field const spanTimeKey = Object.keys(hit._source || {}).find((key) => key.startsWith('span(')); const timeValue = spanTimeKey ? hit._source?.[spanTimeKey] : hit._source?.[timeField]; const requestCount = hit._source?.['request_count'] || hit._source?.['count()'] || 0; - totalCount += requestCount; - if (timeValue && !isNaN(requestCount)) {src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (1)
2414-2436: Consider adding test for statusField edge cases.The test iterates through different
statusFieldconfigurations but doesn't verify the actual query string contains the correct status field. Consider adding an assertion to validate the query builder receives and uses the correct field.for (const config of configs) { mockBuildErrorCountQuery.mockClear(); + mockBuildErrorCountQuery.mockReturnValue( + `source=traces | where ${config.statusField}=2 | stats count() by span(endTime, 5m)` + ); const params = { services: mockServices, cacheKey: `trace-errors:${config.statusField}`, baseQuery: 'source=traces', config: createTraceConfig(config), }; const thunk = executeErrorCountQuery(params); await thunk(mockDispatch, mockGetState, undefined); expect(mockBuildErrorCountQuery).toHaveBeenCalledWith('source=traces', params.config); + // Verify the config contains the expected statusField + expect(params.config.statusField).toBe(config.statusField); }src/plugins/explore/public/components/chart/discover_chart_container.tsx (1)
72-88: Consider wrapping histogram config creation in try-catch.
createHistogramConfigscan throw errors (as seen in its implementation which callsdata.search.showError). If an error occurs, this could crash the component. Consider adding defensive error handling.const actualInterval = useMemo(() => { if (flavorId === ExploreFlavor.Traces && dataset && services?.data && interval) { + try { const histogramConfigs = createHistogramConfigs( dataset, interval, services.data, services.uiSettings, breakdownField, TRACES_CHART_BAR_TARGET ); // Extract interval from configs if available const bucketAggConfig = histogramConfigs?.aggs?.[1] as any; const finalInterval = bucketAggConfig?.buckets?.getInterval()?.expression; return finalInterval || interval || 'auto'; + } catch (error) { + // Fall back to provided interval if config creation fails + return interval || 'auto'; + } } return interval || 'auto'; }, [flavorId, dataset, services, interval, breakdownField]);src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (2)
6-10: Consider extending config for field customization.The
TraceAggregationConfiginterface only includestimeField,interval, andbreakdownField. However, the query builders hardcodestatus.codeanddurationInNanos. For better flexibility with different schemas, consider adding optional field name overrides.export interface TraceAggregationConfig { timeField: string; interval: string; breakdownField?: string; + statusField?: string; // defaults to 'status.code' + durationField?: string; // defaults to 'durationInNanos' + errorStatusValue?: string; // defaults to '2' }This would make the builder more flexible for non-data-prepper schemas in the future.
20-29: Consider adding input validation.If
baseQueryis empty orconfig.timeField/config.intervalare empty strings, the generated queries will be malformed. Consider adding validation.export const buildRequestCountQuery = ( baseQuery: string, config: TraceAggregationConfig ): string => { + if (!baseQuery?.trim()) { + throw new Error('baseQuery is required for building trace aggregation query'); + } + if (!config.timeField?.trim() || !config.interval?.trim()) { + throw new Error('timeField and interval are required in TraceAggregationConfig'); + } + const { timeField, interval, breakdownField } = config; return breakdownField ? `${baseQuery} | stats count() by span(${timeField}, ${interval}), ${breakdownField} | sort ${timeField}` : `${baseQuery} | stats count() by span(${timeField}, ${interval}) | sort ${timeField}`; };src/plugins/explore/public/components/chart/explore_traces_chart.tsx (2)
157-189: Consider usingprepareTraceCacheKeysfor consistency.The trace cache keys are manually constructed on lines 167-169 (
trace-requests:${baseQuery}, etc.), whilequery_actions.tsline 316 usesprepareTraceCacheKeys(query)which callsdefaultPrepareQueryString(query)internally.If
baseQuery(which isdefaultPrepareQueryString(query)) differs from the processed query in any way, this could lead to cache key mismatches. For consistency and maintainability, consider importing and usingprepareTraceCacheKeys.Apply this diff to use the existing helper:
+import { + executeQueries, + defaultPrepareQueryString, + prepareHistogramCacheKey, + prepareTraceCacheKeys, +} from '../../application/utils/state_management/actions/query_actions'; + const onChangeInterval = (newInterval: string) => { dispatch(setInterval(newInterval)); // Clear only chart-related cache keys - const baseQuery = defaultPrepareQueryString(query); const histogramCacheKey = prepareHistogramCacheKey(query, !!breakdownField); + const { requestCacheKey, errorCacheKey, latencyCacheKey } = prepareTraceCacheKeys(query); // Clear histogram and trace aggregation cache keys const chartCacheKeys = [ histogramCacheKey, - `trace-requests:${baseQuery}`, - `trace-errors:${baseQuery}`, - `trace-latency:${baseQuery}`, + requestCacheKey, + errorCacheKey, + latencyCacheKey, ];
293-498: Consider extracting chart rendering logic to reduce duplication.The three chart containers (lines 297-360, 362-431, 433-497) follow nearly identical patterns with only minor differences (chart type, color, error prop). While the current implementation is clear and works correctly, extracting a reusable
TraceChartContainercomponent could improve maintainability.This is a nice-to-have refactor that can be deferred to a future PR if preferred.
Example structure:
const TraceChartContainer = ({ title, ariaLabel, dataTestSubj, chartData, error, chartType, customTheme, timeFieldName, chartTypeName }) => ( <EuiFlexItem className="exploreTracesChart__container"> <section aria-label={ariaLabel} className={...} data-test-subj={dataTestSubj}> <div className={...}> {chartData ? ( <DiscoverHistogram {...} /> ) : error && isFieldMissingError(error) ? ( <EuiCallOut {...} /> ) : error ? ( <EuiCallOut {...} /> ) : ( <EuiLoadingSpinner /> )} </div> </section> </EuiFlexItem> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/plugins/explore/public/application/utils/interfaces.ts(2 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts(15 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts(6 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts(1 hunks)src/plugins/explore/public/components/chart/discover_chart_container.tsx(4 hunks)src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx(1 hunks)src/plugins/explore/public/components/chart/explore_traces_chart.tsx(8 hunks)src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/plugins/explore/public/application/utils/interfaces.ts
- src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/explore/public/components/chart/discover_chart_container.tsx (5)
src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
createHistogramConfigs(26-95)src/plugins/explore/public/application/utils/state_management/constants.ts (1)
TRACES_CHART_BAR_TARGET(14-14)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
prepareTraceCacheKeys(97-104)src/plugins/explore/public/application/utils/interfaces.ts (2)
ProcessedSearchResults(115-118)TracesChartProcessedResults(123-128)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
processTraceAggregationResults(22-146)
src/plugins/explore/public/components/chart/explore_traces_chart.tsx (3)
src/plugins/explore/public/components/chart/timechart_header/timechart_header.tsx (1)
TimechartHeaderBucketInterval(40-44)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (3)
defaultPrepareQueryString(77-86)prepareHistogramCacheKey(91-95)executeQueries(237-421)src/plugins/explore/public/components/chart/histogram/histogram.tsx (1)
DiscoverHistogram(133-425)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (4)
src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
createHistogramConfigs(26-95)src/plugins/explore/public/helpers/get_flavor_from_app_id.ts (1)
getCurrentFlavor(29-34)src/plugins/explore/public/application/utils/state_management/constants.ts (1)
TRACES_CHART_BAR_TARGET(14-14)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (5)
createTraceAggregationConfig(48-58)TraceAggregationConfig(6-10)buildRequestCountQuery(20-29)buildErrorCountQuery(31-38)buildLatencyQuery(40-46)
src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx (1)
src/plugins/explore/public/components/chart/explore_traces_chart.tsx (1)
ExploreTracesChart(131-502)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (7)
histogramResultsProcessor(197-232)defaultPrepareQueryString(77-86)prepareTraceCacheKeys(97-104)executeTraceAggregationQueries(816-858)executeRequestCountQuery(860-882)executeErrorCountQuery(884-906)executeLatencyQuery(908-930)
🪛 Biome (2.1.2)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
[error] 7-7: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (27)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (4)
186-194: Good addition of test helper.The
createTraceConfighelper function promotes DRY testing by providing consistent default config values that can be easily overridden. This is a good pattern for test maintainability.
2075-2213: Comprehensive test coverage for trace aggregation queries.The test suite for
executeTraceAggregationQueriescovers:
- Parallel execution of all three queries
- Individual query failure handling
- Cache key verification
This is a solid foundation for the new trace query functionality.
2216-2328: Good coverage of error scenarios for request count query.Tests cover:
- Successful execution
- Field-not-found errors (semantic check exceptions)
- NO_RESULTS status when empty
This aligns well with the error handling behavior described in the PR objectives.
2569-2605: Integration test validates complex query scenarios well.The test correctly verifies that all three query builders are called with the same complex base query and configuration, ensuring consistency across the RED metrics.
src/plugins/explore/public/components/chart/discover_chart_container.tsx (5)
44-46: Good: Early dataset retrieval for cache key calculations.Moving dataset retrieval earlier ensures it's available for the trace cache key calculations. The comment clearly explains the rationale.
90-96: Dependency array includes variables only used for guard condition.The
datasetandservicesare in the dependency array but are only used for the early-return guard. This is correct behavior for memoization correctness, but consider adding a comment to clarify this is intentional.
111-146: Well-structured processing path differentiation.The clear separation of Traces vs Logs processing paths with appropriate dependencies in useMemo is a good pattern. The dependency array correctly includes all variables used in the computation.
156-164: Verify null checks before rendering.The validation logic correctly ensures:
- Traces: requires
requestChartDatato be present- Logs: requires both
hits.totalandchartDataThis prevents rendering with incomplete data. Good defensive programming.
191-194: Props correctly pass error states and timeFieldName.The new props (
requestError,errorQueryError,latencyError,timeFieldName) enable per-chart error handling inExploreTracesChart, aligning with the PR's error handling objectives.src/plugins/explore/public/components/chart/explore_traces_chart.test.tsx (5)
62-114: Comprehensive test store configuration.The store setup includes all necessary reducers and a complete preloadedState that mirrors the real application state structure. This ensures tests run in a realistic environment.
133-165: Good test for duration field missing error.This test validates:
- The error message title ("Latency Unavailable")
- The field-specific message with field name
- The "required for latency metrics" guidance
- Other charts still render (2 histograms)
This aligns well with the error handling behavior in
ExploreTracesChart.
263-321: Excellent test for multiple simultaneous field errors.This test verifies the component gracefully handles all three charts having field-missing errors simultaneously, showing all three error messages and rendering no charts. This is an important edge case.
323-354: Good coverage of non-field-missing errors.This test ensures generic server errors display the actual error message rather than the field-missing template, and don't cause infinite loading states.
356-372: Normal rendering path verified.The final test confirms that when no errors are present and all chart data is provided, all three histograms render without error messages or loading indicators.
src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (3)
31-38: Hard-coded error condition limits schema compatibility.The PR mentions this is "currently compatible only with the data-prepper schema." The hardcoded
status.code=2will fail for other tracing schemas. Consider either:
- Making this configurable via the config object
- Adding a clear comment about the schema dependency
Since the PR explicitly mentions data-prepper only compatibility, this may be intentional. Please confirm this limitation is documented and acceptable for this PR scope.
40-46: Null filtering withisnotnullis good defensive practice.The
where isnotnull(durationInNanos)clause prevents aggregation errors when the duration field is missing from some documents. This is a good defensive pattern.
48-58:createTraceAggregationConfigis a simple factory function.While this works, it's essentially just object construction. The value is in providing a consistent creation pattern and potential future extension point.
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (6)
9-9: LGTM: Imports are well-organized.The new imports for trace aggregation, flavor detection, and UI settings are appropriately added and used throughout the file.
Also applies to: 49-58
97-104: LGTM: Cache key generation follows existing patterns.The function correctly generates distinct cache keys for the three trace metrics, following the same pattern as
prepareHistogramCacheKey.
860-930: LGTM: Individual trace query thunks follow consistent patterns.The three query thunks (
executeRequestCountQuery,executeErrorCountQuery,executeLatencyQuery) are well-structured and follow a consistent pattern. They properly delegate toexecuteQueryBasefor common logic while using their respective query builders.
297-359: Review dataset retrieval safety—existing guard may address concerns.The code includes a conditional check on line 304 (
if (dataset?.timeFieldName)) that prevents trace queries from executing if the dataset or its timeFieldName is missing. Verify whether this guard is sufficient for error handling, or if explicit error logging should be added when queries are skipped. Additionally, confirm the return type ofgetCurrentFlavor—if it cannot return null based on its implementation, that concern can be removed.
197-232: Verify that all callers ofhistogramResultsProcessorhave been updated.The function signature has been modified to include
interval: stringanduiSettings: IUiSettingsClientas new parameters. All call sites must pass these parameters to avoid runtime errors. Ensure theHistogramDataProcessortype definition was also updated to reflect this signature change.
816-858: Verify ifexecuteTraceAggregationQueriesis actively used in the codebase.The orchestrator function
executeTraceAggregationQueriesappears to be defined but may not be invoked anywhere. If this function isn't used, consider removing it to reduce code complexity. Additionally, verify that the cache keys used in this orchestrator (trace-requests:${baseQuery}, etc.) are consistent with the output ofprepareTraceCacheKeysif the processed query differs from the rawbaseQueryparameter.src/plugins/explore/public/components/chart/explore_traces_chart.tsx (4)
53-103: LGTM: Error detection and messaging utilities are well-implemented.The error handling utilities correctly detect field-missing errors and provide user-friendly, localized messages. The dynamic time field handling (lines 72-78) is especially good for flexibility.
One minor observation: the regex pattern on line 62 assumes a specific error format. If the error format changes, this could silently fail and return "unknown field". Consider adding logging or fallback handling if this becomes a maintenance concern.
105-129: LGTM: Interface changes support error handling.The new
ChartQueryErrorinterface and additional props (requestError,errorQueryError,latencyError,timeFieldName) are well-defined and necessary for the error handling functionality.
218-250: Verify interval selector visibility logic.The
hideIntervalSelector={true}is set on line 247 in the collapsed view (whenshowHistogramis false). When the histogram is expanded (line 218-232), a custom header is shown without an interval selector.This means users can only change the interval when the charts are collapsed. This seems counterintuitive - typically, you'd want the interval selector visible when viewing the expanded charts. Please verify this is the intended UX behavior.
48-51: The comment about PPL not supporting milliseconds is incorrect — milliseconds are supported.OpenSearch PPL explicitly supports milliseconds (ms) as a time interval unit in both span() operations and date_histogram aggregations with fixed_interval. The filtering logic that removes 'ms' from tracesIntervalOptions appears to be based on a false premise and should be reconsidered.
Likely an incorrect or invalid review comment.
.../public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
Show resolved
Hide resolved
.../public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
Show resolved
Hide resolved
.../public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
Show resolved
Hide resolved
.../public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
Show resolved
Hide resolved
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (2)
99-101: Empty catch block swallows errors silently.The empty catch block at lines 99-101 (and also at lines 141-143) makes debugging difficult if histogram config generation or processing fails. Consider logging the error or providing fallback behavior.
Apply this diff to improve error visibility:
} catch (error) { - // Fall back to hardcoded format if histogram config fails + console.warn('Failed to create histogram configs, using fallback format:', error); }And for the outer catch block:
} catch (error) { - // Error during processing + console.error('Error processing trace aggregation results:', error); }
236-236: Unused variable can be removed.The
totalCountvariable is incremented but never used. Consider removing it to reduce clutter.Apply this diff:
if (results.hits?.hits) { const dataMap = new Map<number, number>(); - let totalCount = 0; actualIntervalMs = extractPPLIntervalMs(results, intervalMs);And remove line 265:
const requestCount = hit._source?.['request_count'] || hit._source?.['count()'] || 0; - totalCount += requestCount; - if (timeValue && !isNaN(requestCount)) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (2)
src/plugins/explore/public/application/utils/interfaces.ts (3)
ChartData(44-53)BucketInterval(58-63)TracesChartProcessedResults(123-128)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
defaultResultsProcessor(111-142)
🪛 Biome (2.1.2)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
[error] 7-7: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Build min release artifacts on macOS x64
🔇 Additional comments (6)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (6)
148-185: LGTM!The interval conversion logic correctly handles various formats including single-character intervals, standard intervals, and edge cases like 'auto'. The defaults are reasonable.
187-219: LGTM!The PPL interval extraction logic is well-structured with proper fallback handling when span fields or intervals are missing.
240-256: Good fix for potential infinite loop!The guard against invalid
actualIntervalMsvalues properly addresses the past review comment about potential infinite loops. The early return with a valid ChartData structure is appropriate.
271-271: Good fixes for data accumulation and interval consistency!Lines 271 and 348 now properly accumulate counts instead of overwriting, addressing the past review comment. Lines 294-296 (and similarly in other processors) now consistently use
actualIntervalMsinstead ofintervalMsin the ordered object, addressing another past review comment.Also applies to: 292-296
303-379: LGTM!The error count processing correctly mirrors the request count logic with proper accumulation, interval guards, and consistent use of
actualIntervalMs. All past review comments have been addressed.
395-395: Excellent fix for latency averaging!The latency processing now properly tracks both sum and count per bucket (line 395, 428-429) and calculates the average correctly (line 440), fully addressing the past review comment about overwriting values.
Also applies to: 428-429, 440-440
| */ | ||
|
|
||
| import moment from 'moment'; | ||
| import { DataView } from '../../../../../../../data/common'; |
There was a problem hiding this comment.
DataView import shadows the global property.
The imported DataView type shadows the built-in browser global DataView (used for TypedArray buffers). While this is unlikely to cause issues in practice since the browser's DataView is rarely used in this context, consider importing with an alias to avoid confusion.
Apply this diff to use an alias:
-import { DataView } from '../../../../../../../data/common';
+import { DataView as DatasetView } from '../../../../../../../data/common';Then update all usages of DataView to DatasetView throughout the file (lines 26, 223, 305, 383).
Based on static analysis hints.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (2.1.2)
[error] 7-7: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🤖 Prompt for AI Agents
In
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
around line 7, the imported DataView shadows the browser global DataView; change
the import to use an alias (e.g., import { DataView as DatasetView } from
'../../../../../../../data/common') and then update all occurrences of DataView
in this file to DatasetView at the referenced lines (26, 223, 305, 383) so the
module type no longer conflicts with the global.
|
|
| requestAggResults: ISearchResult | null, | ||
| errorAggResults: ISearchResult | null, | ||
| latencyAggResults: ISearchResult | null, | ||
| dataset: DataView, | ||
| interval: string, | ||
| timeField: string = 'endTime', | ||
| dataPlugin?: DataPublicPluginStart, | ||
| rawInterval?: string, | ||
| uiSettings?: IUiSettingsClient |
There was a problem hiding this comment.
might be better to use object as argument, so order doesn't matter when adding new args in the future
There was a problem hiding this comment.
Created ProcessTraceAggregationParams interface and updated processTraceAggregationResults to use a single object parameter. This makes parameter order irrelevant and future parameter additions easier. All call sites (production and tests) have been updated to use the named parameter syntax.
| let minTime: number; | ||
| let maxTime: number; | ||
|
|
||
| if (dataPlugin) { |
There was a problem hiding this comment.
why wouldn't dataPlugin be always available?
There was a problem hiding this comment.
Made dataPlugin a required parameter since it's always available in production. Removed unnecessary if (dataPlugin) checks and simplified the time range logic. Tests were updated to always provide the mock dataPlugin.
| const match = interval.match(/^(\d+)([smhd])$/); | ||
| if (!match) { | ||
| if (interval === 'auto') { | ||
| return 30000; // 30 seconds for auto intervals |
There was a problem hiding this comment.
Auto intervals are now dynamically calculated based on the time range using histogram configs (same approach as logs). The hardcoded 30s fallback was removed. When interval is "auto", we now use createHistogramConfigs to calculate the optimal interval based on the selected time range, then convert that to milliseconds. The interval selection is disabled at the moment pending bugfix but will work when re-enabled if we decide to do that now.
| ); | ||
| }); | ||
|
|
||
| export const executeTraceAggregationQueries = createAsyncThunk< |
There was a problem hiding this comment.
would be better to move them out from query_actions, so if traces query ever change, we don't need to change the shared query_actions file
There was a problem hiding this comment.
Moved all trace-specific query actions (executeTraceAggregationQueries, executeRequestCountQuery, executeErrorCountQuery, executeLatencyQuery, and prepareTraceCacheKeys) to a new dedicated file trace_query_actions.ts. This ensures future trace query changes won't affect the shared query_actions file.
Signed-off-by: Adam Tackett <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
7-7: Avoid shadowing the globalDataViewproperty.This was flagged in a previous review. Consider using an alias to avoid confusion with the browser's built-in
DataView.-import { DataView } from '../../../../../../../data/common'; +import { DataView as DatasetView } from '../../../../../../../data/common';Then update usages at lines 26, 233, 315, 393.
🧹 Nitpick comments (5)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (1)
11-11: Avoid shadowing the globalDataViewproperty.The import shadows the browser's built-in
DataView(used for TypedArray buffers). While unlikely to cause issues in tests, using an alias improves clarity.-import { DataView } from '../../../../../../../data/common'; +import { DataView as DataViewType } from '../../../../../../../data/common';Then update usages (lines 14, 22) to
DataViewType. Based on static analysis hints.src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.test.ts (1)
58-58: Avoid shadowing the globalDataViewproperty.Same issue as in the processor test file - consider using an alias.
-import { Query, DataView } from 'src/plugins/data/common'; +import { Query, DataView as DataViewType } from 'src/plugins/data/common';Based on static analysis hints.
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
288-350: Consider adding cache checks for trace queries.The data table and histogram queries check if results already exist before dispatching (lines 253-258). The trace queries dispatch unconditionally which could cause redundant queries on re-renders.
+ const requestQueryStatus = state.queryEditor.queryStatusMap[requestCacheKey]; + const errorQueryStatus = state.queryEditor.queryStatusMap[errorCacheKey]; + const latencyQueryStatus = state.queryEditor.queryStatusMap[latencyCacheKey]; + + const needsRequestQuery = + !results[requestCacheKey] || + requestQueryStatus?.status === QueryExecutionStatus.UNINITIALIZED; + const needsErrorQuery = + !results[errorCacheKey] || + errorQueryStatus?.status === QueryExecutionStatus.UNINITIALIZED; + const needsLatencyQuery = + !results[latencyCacheKey] || + latencyQueryStatus?.status === QueryExecutionStatus.UNINITIALIZED; + - promises.push( - dispatch( - executeRequestCountQuery({ + if (needsRequestQuery) { + promises.push( + dispatch( + executeRequestCountQuery({Apply similar pattern for error and latency queries.
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts (2)
127-128: Consider hoisting the dynamic import forindexPatternsutilities.The dynamic import
await import('../../../../../../data/public')is executed on every query invocation. While the module is likely cached after the first import, moving this to a module-level import or a lazy-initialized singleton would be cleaner and avoid the async overhead on hot paths.+import { indexPatterns as indexPatternUtils } from '../../../../../../data/public'; // In executeTraceQueryBase: - const { indexPatterns: indexPatternUtils } = await import('../../../../../../data/public'); - const { isDefault } = indexPatternUtils; + const { isDefault } = indexPatternUtils;
316-336: Consider using a more specific return type instead ofany.The three individual query thunks (
executeRequestCountQuery,executeErrorCountQuery,executeLatencyQuery) useanyas their return type. UsingISearchResult | undefinedwould provide better type safety and align with the actual return type ofexecuteTraceQueryBase.export const executeRequestCountQuery = createAsyncThunk< - any, + ISearchResult | undefined, { services: ExploreServices; cacheKey: string; baseQuery: string; config: TraceAggregationConfig; }, { state: RootState } >('query/executeRequestCountQuery', async ({ services, cacheKey, baseQuery, config }, thunkAPI) => {Apply the same pattern to
executeErrorCountQueryandexecuteLatencyQuery.Also applies to: 341-361, 366-386
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts(11 hunks)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts(4 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.test.ts(1 hunks)src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts(1 hunks)src/plugins/explore/public/components/chart/discover_chart_container.test.tsx(10 hunks)src/plugins/explore/public/components/chart/discover_chart_container.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/plugins/explore/public/components/chart/discover_chart_container.tsx (3)
src/plugins/explore/public/application/utils/state_management/constants.ts (1)
TRACES_CHART_BAR_TARGET(14-14)src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts (1)
prepareTraceCacheKeys(41-48)src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
processTraceAggregationResults(34-159)
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts (5)
prepareTraceCacheKeys(41-48)executeTraceAggregationQueries(269-311)executeRequestCountQuery(316-336)executeErrorCountQuery(341-361)executeLatencyQuery(366-386)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (2)
extractPPLIntervalMs(197-229)processTraceAggregationResults(34-159)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (5)
src/plugins/explore/public/application/utils/interfaces.ts (3)
ChartData(44-53)BucketInterval(58-63)TracesChartProcessedResults(123-128)src/plugins/data/public/index.ts (2)
DataView(281-281)DataPublicPluginStart(606-606)src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
defaultResultsProcessor(102-133)src/plugins/explore/public/components/chart/utils/create_histogram_configs.ts (1)
createHistogramConfigs(26-95)src/dev/jest/setup/mocks.js (1)
moment(53-53)
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts (3)
src/plugins/explore/public/application/utils/languages/ppl/default_prepare_ppl_query/default_prepare_ppl_query.ts (1)
defaultPreparePplQuery(14-16)src/plugins/explore/public/types.ts (1)
ExploreServices(137-195)src/plugins/explore/public/application/utils/state_management/actions/trace_aggregation_builder.ts (4)
TraceAggregationConfig(6-10)buildRequestCountQuery(20-29)buildErrorCountQuery(31-38)buildLatencyQuery(40-46)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (1)
src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (1)
histogramResultsProcessor(188-223)
src/plugins/explore/public/components/chart/discover_chart_container.test.tsx (1)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (1)
processTraceAggregationResults(34-159)
🪛 Biome (2.1.2)
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.test.ts
[error] 58-58: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts
[error] 11-11: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts
[error] 7-7: Do not shadow the global "DataView" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (67)
- GitHub Check: Run functional tests on Windows (ciGroup4)
- GitHub Check: Run functional tests on Windows (ciGroup8)
- GitHub Check: Run functional tests on Windows (ciGroup11)
- GitHub Check: Run functional tests on Windows (ciGroup9)
- GitHub Check: Run functional tests on Windows (ciGroup7)
- GitHub Check: Run functional tests on Windows (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup13)
- GitHub Check: Run functional tests on Windows (ciGroup6)
- GitHub Check: Run functional tests on Windows (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup10)
- GitHub Check: Run functional tests on Linux (ciGroup12)
- GitHub Check: Run functional tests on Windows (ciGroup2)
- GitHub Check: Run functional tests on Windows (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup10)
- GitHub Check: Run functional tests on Windows (ciGroup1)
- GitHub Check: Run functional tests on Linux (ciGroup9)
- GitHub Check: Run functional tests on Linux (ciGroup6)
- GitHub Check: Run functional tests on Linux (ciGroup7)
- GitHub Check: Run functional tests on Linux (ciGroup2)
- GitHub Check: Run functional tests on Linux (ciGroup11)
- GitHub Check: Run functional tests on Linux (ciGroup8)
- GitHub Check: Run functional tests on Linux (ciGroup5)
- GitHub Check: Run functional tests on Linux (ciGroup4)
- GitHub Check: Run functional tests on Linux (ciGroup3)
- GitHub Check: Run functional tests on Linux (ciGroup1)
- GitHub Check: Run cypress tests (osd:ciGroup17Explore)
- GitHub Check: Run cypress tests (osd:ciGroup15)
- GitHub Check: Run cypress tests (osd:ciGroup13Explore)
- GitHub Check: Run cypress tests (osd:ciGroup13)
- GitHub Check: Run cypress tests (osd:ciGroup15Explore)
- GitHub Check: Run cypress tests (osd:ciGroup16Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14Explore)
- GitHub Check: Run cypress tests (osd:ciGroup12)
- GitHub Check: Run cypress tests (osd:ciGroup12Explore)
- GitHub Check: Run cypress tests (osd:ciGroup10Slow)
- GitHub Check: Run cypress tests (osd:ciGroup10Explore)
- GitHub Check: Run cypress tests (osd:ciGroup14)
- GitHub Check: Build min release artifacts on Windows x64
- GitHub Check: Run cypress tests (osd:ciGroup11)
- GitHub Check: Run cypress tests (osd:ciGroup10Fast)
- GitHub Check: Build min release artifacts on macOS x64
- GitHub Check: Run cypress tests (osd:ciGroup6)
- GitHub Check: Run cypress tests (osd:ciGroup4)
- GitHub Check: Build min release artifacts on macOS ARM64
- GitHub Check: Run cypress tests (osd:ciGroup8)
- GitHub Check: Run cypress tests (osd:ciGroup9)
- GitHub Check: Run cypress tests (osd:ciGroup5)
- GitHub Check: Run cypress tests (osd:ciGroup3)
- GitHub Check: Run cypress tests (osd:ciGroup7)
- GitHub Check: Build and Verify on Windows (ciGroup1)
- GitHub Check: Build min release artifacts on Linux ARM64
- GitHub Check: Run cypress tests (osd:ciGroup2)
- GitHub Check: Build min release artifacts on Linux x64
- GitHub Check: Run cypress tests (osd:ciGroup1)
- GitHub Check: Run plugin functional tests on Windows
- GitHub Check: Run plugin functional tests on Linux
- GitHub Check: Build and Verify on Windows (ciGroup4)
- GitHub Check: Build and Verify on Windows (ciGroup3)
- GitHub Check: Build and Verify on Windows (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup3)
- GitHub Check: Build and Verify on Linux (ciGroup4)
- GitHub Check: Build and Verify on Linux (ciGroup2)
- GitHub Check: Build and Verify on Linux (ciGroup1)
- GitHub Check: Lint and validate
- GitHub Check: lighthouse
- GitHub Check: bundle-analyzer
🔇 Additional comments (39)
src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.test.ts (5)
17-60: LGTM!Test setup is well-structured with appropriate mocks for
DataViewandDataPlugin. ThecreateMockSearchResulthelper provides a clean, reusable way to create test data.
62-105: LGTM!Comprehensive test coverage for
extractPPLIntervalMsincluding all time units (s/m/h/d), fallback behavior, and empty results handling. The parameterized approach withtestCasesis clean and maintainable.
107-233: LGTM!Good coverage of
processTraceAggregationResultsfor individual and combined result types. Tests correctly validate output structure includingyAxisLabelvalues andbucketInterval.
235-459: LGTM!Thorough error handling tests covering malformed data, invalid timestamps, missing fields, null metrics, and edge cases like extremely large time ranges. The upper bound check at line 434 is a good safeguard against performance issues.
461-551: LGTM!Excellent coverage of field-missing scenarios that align with the PR's error-handling behavior. Tests verify that charts are still created with proper labels and zero values when expected fields are missing.
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.test.ts (6)
6-148: LGTM!Comprehensive mock setup necessary for testing async thunks with Redux Toolkit. The use of
jest.requireActualfor partial mocks of moment is appropriate.
149-269: LGTM!Well-structured test setup with
createTraceConfighelper for easy configuration overrides. The mock services setup is comprehensive and covers all necessary dependencies.
271-294: LGTM!Regression test verifies that cache keys correctly include the base query string for all three trace metrics.
334-435: LGTM!Good test coverage for
executeTraceAggregationQueriesincluding parallel execution, individual query failures, and cache key verification. The custom mock dispatch approach for tracking thunk calls is appropriate.
437-788: LGTM!Thorough test coverage for individual trace query actions. Tests verify success paths, error handling with proper status dispatch, and configuration variations for different field names.
790-877: LGTM!Integration tests provide good end-to-end coverage including complex queries, abort handling, and time field variations. The abort test correctly verifies the status is reset to
UNINITIALIZED.src/plugins/explore/public/components/chart/discover_chart_container.tsx (6)
18-30: LGTM!New imports support the trace aggregation functionality including per-trace cache keys, histogram configuration, and the traces chart bar target constant.
44-46: LGTM!Dataset context is correctly moved earlier to support cache key calculations. The
actualIntervalcomputation appropriately uses histogram configs for Traces flavor with proper fallbacks.Also applies to: 72-88
90-105: LGTM!Trace cache keys and per-query error states are correctly handled with appropriate null checks for non-Traces scenarios. This aligns with the PR's distinct error handling UI.
111-146: LGTM!Processing logic correctly branches based on flavor. Traces use
processTraceAggregationResultswith all three aggregation results, while Logs continue usinghistogramResultsProcessor(now withuiSettings). The dependency array is complete.
152-164: LGTM!Result validation appropriately differs by flavor. Traces only require
requestChartData(allowing error/latency to fail independently), while Logs require bothchartDataand non-zero hits.
179-199: LGTM!
ExploreTracesChartreceives the new error props (requestError,errorQueryError,latencyError) andtimeFieldNameto support per-chart error display and proper axis labeling.src/plugins/explore/public/application/utils/state_management/actions/query_actions.ts (2)
49-58: LGTM!New imports support the Traces flavor integration including flavor detection, trace aggregation config, and the individual trace query actions.
188-201: LGTM!The
uiSettingsparameter addition tohistogramResultsProcessorenables proper interval calculation via histogram configs. The call site indiscover_chart_container.tsx(line 134) is correctly updated to pass this parameter.src/plugins/explore/public/application/utils/state_management/actions/processors/trace_aggregation_processor.ts (7)
15-64: LGTM!Well-designed interfaces following the past review suggestion to use object parameters. The default empty structure and early return for null results are appropriate.
66-115: LGTM!Robust interval calculation with proper fallbacks. Uses histogram configs when available (addressing past review about dynamic interval calculation) and falls back gracefully when config creation fails.
161-195: LGTM!
convertIntervalToMshandles edge cases well including unit-only intervals. The recursive call for unit-only formats is clean, and the default fallback to 5 minutes is reasonable.
197-229: LGTM!
extractPPLIntervalMsrobustly extracts interval information from PPL span fields with appropriate null checks and fallback behavior.
231-311: LGTM!
processRequestCountDataproperly addresses previous review feedback:
- Line 250-266: Guard against infinite loops when
actualIntervalMsis invalid- Line 281: Correctly accumulates request counts per bucket
- Lines 302-309: Uses
actualIntervalMsconsistently in theorderedobject
313-389: LGTM!
processErrorCountDataproperly addresses previous review feedback:
- Line 331-347: Guard against infinite loops
- Line 358: Now correctly accumulates error counts instead of overwriting
- Lines 380-387: Uses
actualIntervalMsconsistently
391-471: LGTM!
processLatencyDataproperly addresses previous review feedback:
- Line 405: Uses
{sum, count}map for proper averaging- Lines 438-439: Correctly accumulates sum and count
- Line 450: Computes proper average (
sum/count)- Guard against infinite loops and interval consistency are maintained
src/plugins/explore/public/application/utils/state_management/actions/query_actions.test.ts (4)
31-49: LGTM! Mock structure correctly reflects the updatedcreateHistogramConfigWithIntervalreturn shape.The mock now returns both
histogramConfigs(withtoDslandaggs) and top-level fields (aggs,effectiveInterval,finalInterval,fromDate,toDate,timeFieldName), which aligns with the updated histogram config creation contract.
147-166: LGTM! Trace aggregation builder and flavor mocks added.The mocks provide simplified implementations that return expected query string formats, enabling tests to validate trace-related wiring without complex dependencies.
705-711: LGTM! Test correctly updated to passuiSettingstohistogramResultsProcessor.This aligns with the updated function signature in
query_actions.tsthat now requiresuiSettingsas the sixth parameter.
713-718: LGTM! Assertion correctly verifiesuiSettingsis passed tocreateHistogramConfigs.The test validates that the histogram config creation receives the required
uiSettingsparameter.src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts (3)
1-21: LGTM! Clean module setup with appropriate imports.The imports cover all necessary dependencies for trace query execution, including the trace aggregation builders and Redux toolkit utilities.
23-48: LGTM! Abort controller management and cache key preparation are well-structured.The module-level
activeTraceQueryAbortControllersmap enables proper cleanup of concurrent queries by cacheKey. TheprepareTraceCacheKeysfunction generates consistent, prefixed cache keys for the three trace query types.
269-311: Parallel query execution with fail-fast behavior.Using
Promise.allwith.unwrap()means if any of the three queries fails, the entireexecuteTraceAggregationQuerieswill reject immediately. This is likely the desired behavior for RED metrics where partial data may not be useful, but verify this aligns with the intended UX (e.g., should the UI show partial results if only one query fails?).src/plugins/explore/public/components/chart/discover_chart_container.test.tsx (6)
87-93: LGTM! Mock for trace query actions correctly set up.The mock returns consistent cache keys that match the preloaded state data, enabling proper test execution for trace-related scenarios.
103-121: LGTM! Query actions mock extended with trace query executors.The mock includes the new
executeRequestCountQuery,executeErrorCountQuery, andexecuteLatencyQueryfunctions returning pending action shapes, which is appropriate for testing component behavior without triggering actual async logic.
124-135: LGTM! Mock updated to use newprocessTraceAggregationResultsinterface.The mock correctly reflects the renamed processor function and returns the expected result shape with
requestChartData,errorChartData, andlatencyChartData.
137-147: LGTM! Utility mock provides expected histogram config structure.The mock for
createHistogramConfigWithIntervalreturns the expected shape withhistogramConfigs, interval values, and time range fields.
218-241: LGTM! Trace query results added to preloaded state.The preloaded state now includes mock results for all three trace query cache keys (
trace-requests:,trace-errors:,trace-latency:), enabling comprehensive test coverage for trace chart rendering scenarios.
316-331: LGTM! Test updated to use new processor name.The test correctly references
processTraceAggregationResultsinstead of the previoustracesHistogramResultsProcessor, and validates that returningnullforrequestChartDataresults in no chart being rendered.
src/plugins/explore/public/application/utils/state_management/actions/trace_query_actions.ts
Show resolved
Hide resolved
@TackAdam I would recommend to display both as red error messages. In either scenario, it cannot generate a visualization. A yellow warning may be suitable for situations like a visualization with partial data. |
3 participants
Description
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jestyarn test:jest_integrationSummary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Style
✏️ Tip: You can customize this high-level summary in your review settings.