Skip to content

fix: exclude .keyword sub-fields from trace-to-log correlation - #12749

Open
TackAdam wants to merge 1 commit into
opensearch-project:mainfrom
TackAdam:fix/correlation-keyword-ppl-field
Open

TackAdam wants to merge 1 commit into
opensearch-project:mainfrom
TackAdam:fix/correlation-keyword-ppl-field

Conversation

@TackAdam

Copy link
Copy Markdown
Collaborator

Description

Fixes trace-to-log correlation failing when a correlated field is mapped to a .keyword multi-field sub-field.

PPL cannot reference a .keyword multi-field in a where clause — the query fails with AssertionError:

POST _plugins/_ppl
{"query":"source = fluent-bit* | where log_processed.trace_id.keyword = 'abc' | head 10"}
-> {"error":{"reason":"Invalid Query","details":"","type":"AssertionError"},"status":400}

The same query against the base field (log_processed.trace_id) succeeds. The trace-to-log correlation UI let a user pick the .keyword sub-field for traceId/spanId/serviceName, stored it in the log dataset's schemaMappings.otelLogs, and later interpolated it verbatim into the PPL where clause — so adding such a dataset to a correlation caused "no related logs" for that span.

Changes

  • Correlation field selector (field_mapping_editor.tsx): exclude multi-field sub-fields (field.subType.multi, i.e. .keyword) from the dropdown options, so they can no longer be selected. Extracted getCorrelationFieldOptions() for direct unit testing.
  • PPL query build (ppl_request_logs.tsx): add a small toPplFieldName() helper that strips a trailing .keyword, and use it for the traceId field. This defensively fixes correlations that were already saved with a .keyword field before this change.
  • Explore-logs URL builder (url_builder.ts): strip .keyword from the traceId/spanId field names used in the generated PPL query, and in the client-side filterLogsBySpanId lookup (the PPL response returns the base field).

Scope

This targets the trace-to-log correlation path, which is where the .keyword field reaches PPL. Both the selection surface (prevention) and the query-build/URL surfaces (fix for existing mappings) are covered.

Issues Resolved

N/A

Screenshot

N/A — behavior fix; the only UI change is that .keyword sub-fields no longer appear in the correlation field dropdown.

Testing the changes

  • yarn test:jest for the three affected suites (correlation field-mapping editor, PPL logs request, logs URL builder) — all pass, with added coverage for: excluding .keyword from the dropdown options, stripping .keyword in the PPL query, and stripping it in the generated Explore-logs URL.
  • yarn typecheck passes.

Manual: configure a trace-to-log correlation, confirm .keyword fields are no longer offered in the field dropdown, and that a span with a previously-saved .keyword mapping now resolves related logs.

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

… PPL queries

Signed-off-by: Adam Tackett <tackadam@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Validate field name before PPL interpolation

The toPplFieldName helper only strips a trailing .keyword but does not escape the
field name for safe use in the PPL where clause. Since the mapped field name flows
directly into an interpolated query string in both fetchTraceLogsByTraceId and
buildExploreLogsUrl, a malformed or malicious mapping (e.g. containing spaces,
quotes, or backticks) could break the query or enable injection. Consider
validating/escaping the field name identifier before interpolation.

src/plugins/explore/public/application/pages/traces/trace_details/server/ppl_request_logs.tsx [17]

-export const toPplFieldName = (fieldName: string): string => fieldName.replace(/\.keyword$/, '');
+export const toPplFieldName = (fieldName: string): string => {
+  const stripped = fieldName.replace(/\.keyword$/, '');
+  if (!/^[A-Za-z_][A-Za-z0-9_.]*$/.test(stripped)) {
+    throw new Error(`Invalid PPL field name: ${fieldName}`);
+  }
+  return stripped;
+};
Suggestion importance[1-10]: 3

__

Why: The suggestion raises a reasonable defensive concern about PPL query interpolation, but field names come from controlled schema mappings rather than untrusted user input, so the security impact is limited. Adding strict validation could also break legitimate field names with unusual characters.

Low

@TackAdam TackAdam changed the title fix(explore): exclude .keyword sub-fields from trace-to-log correlation fix: exclude .keyword sub-fields from trace-to-log correlation Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

🔗 Workflow run · commit fe0705a405fb9ddcad906ed2d97c7ad52436cab4

❌ 1 Jest Test Failure(s)

📄 junit-jest-integration-Windows/TEST-Jest Integration Tests.xml

❌ Options authRequired optional User has access to a route if auth mechanism not registered (0.067s)

Jest Integration Tests.src\core\server\http\integration_tests

Error: expect(received).rejects.toThrow(expected)

Expected substring: "socket hang up"
Received message:   "read ECONNRESET"

    at Object.toThrow (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\node_modules\expect\build\index.js:2155:20)
    at Object.toThrow (D:\a\OpenSearch-Dashboards\OpenSearch-Dashboards\src\core\server\http\integration_tests\router.test.ts:446:38)
    at processTicksAndRejections (node:internal/process/task_queues:103:5)

1 failure(s) across 1 suite(s). Full XML reports are in the junit-jest-* artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant