Add advanced setting for default log columns in trace samples#275365
Add advanced setting for default log columns in trace samples#275365MiriamAparicio merged 12 commits into
Conversation
MiriamAparicio
added
Team:obs-presentation
|
Pinging @elastic/obs-presentation-team (Team:obs-presentation) |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One inline comment on a code/comment mismatch in the kbn-saved-search-component effect dependency array. The rest of the change (column resolution helper with tests, settings subscription, URL-sync dedup) looks coherent and well-covered.
Generated by Claude Reviewer for issue #275365 · 124.4 AIC · ⌖ 12.1 AIC · ⊞ 3.9K
c3457dd to
62bb3f9
Compare
d4171c1 to
e6add87
Compare
There was a problem hiding this comment.
One inline finding: setting-derived default columns can be auto-persisted to the logsColumns URL param on tab view, which then suppresses live setting updates and prevents clearing the setting from resetting affected users. The prior dep-array/comment mismatch is resolved, and the rest of the change (column-resolution helper with tests, settings subscription, URL-sync dedup) looks coherent.
Generated by Claude Reviewer for issue #275365 · 120.6 AIC · ⌖ 10.5 AIC · ⊞ 3.9K
sbelastic
left a comment
sbelastic
left a comment
There was a problem hiding this comment.
Took a long while because of testing, just found one bug/improvement.
After changing the settings, and going back, the fields don't have the new values, you need to leave the page completely and go back to see the new fields.
bug1.mp4
1aa1e7b to
156bffb
Compare
|
Hi @sbelastic , thanks for the review, fixed here 156bffb Screen.Recording.2026-06-30.at.16.53.41.mov |
MiriamAparicio
added
the
ci:project-deploy-observability
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
Kibana Exploratory Testing — execution reportRun Metadata
FindingsStep 1: Verify base stream data via Elasticsearch APISeverity: warning
Step 5: Test significant events preview APISeverity: bug
Step 6: Test edge case - Simulate processing with zero documentsSeverity: warning Step 9: Test failure store samples APISeverity: warning Step 10: Cross-verify API data with UISeverity: info
Additional detailsScreenshotsScreenshots are available in the workflow artifacts.
Workflow run: View workflow run |
1458756 to
e1752fc
Compare
|
/oblt-deploy |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
sbelastic
left a comment
sbelastic
left a comment
There was a problem hiding this comment.
Changes LGTM, awesome job :)
weltenwort
left a comment
weltenwort
left a comment
There was a problem hiding this comment.
It works as specified.
I'm impressed by how you found a way to intercept the "no columns" state without rendering the summary column temporarily. At the same time I question the choice of disallowing that state (see below). 😇
As I'm not aware of the finer-grained product discussions that happened around this feature, I'll leave it to you to judge whether it's worth fine-tuning that aspect.
| /** | ||
| * Called synchronously when the embeddable column state changes. | ||
| * Return column names to substitute Discover default/Summary columns; return | ||
| * undefined to keep the emitted columns. | ||
| */ | ||
| resolveColumnsOnChange?: (columns: string[] | undefined) => string[] | undefined; |
There was a problem hiding this comment.
As a reader this might be confusing at this point and lead to questions like:
- What does the "default" have to do with it? Does it use the columns I return or not?
- Why return undefined and not just
columnsif I want to keep all of them?
Do you think anything can be done about this? For example, could this just be "use whatever columns this returns"? Do we need the undefined case?
There was a problem hiding this comment.
Good point , I can see how this can be misleading here. undefined doesn't mean "use defaults", it means "don't substitute, keep whatever the embeddable just emitted". A returned array means "replace the emitted columns with these".
We only use this in APM to intercept Discover's internal "no columns to Summary (_source)" transition and swap in the admin configured default columns instead. The word "default" in the comment refers to Discover's Summary state, not the APM advanced setting.
Happy to improve this in a follow-up, i.e. rename to something like substituteColumnsOnChange and document the two return semantics more explicitly. Didn't want to bikeshed the shared component API in this PR, but agree it's worth clarifying.
Maybe something like
/**
* Called when the embeddable column state changes.
* Return `undefined` to keep the emitted columns unchanged.
* Return a column list to replace Discover's emitted columns (e.g. substitute Summary for app-specific columns).
*/
WDYT?
|
|
||
| const SOURCE_COLUMN = '_source'; | ||
|
|
||
| export function isDiscoverDefaultLogColumns(columns?: string[]): boolean { |
There was a problem hiding this comment.
It makes me uneasy that this contains so many implicit assumptions and duplicate knowledge about how the saved search embeddable handles its column state. I don't see an obvious way to improve that, though, short of refactoring the unified data table.
Maybe the root of the problem is that we're conflating the "show the summary" state with the "show the default APM columns" state. Why are we disallowing the user to choose to remove all columns to see the summary?
There was a problem hiding this comment.
I don't think we're blocking "remove all columns" in the sense of resetting the table, users can still remove columns they added. The tricky part is that Discover treats "zero columns" as Summary (_source) internally (getDisplayedColumns), so "remove everything" and "show Summary" are the same state inside the embeddable.
When an admin setting is configured, we intentionally substitute that Summary transition with the setting columns instead, that was discussed with product for this. If the setting is empty, Summary is still reachable.
Agree the helper duplicates some Discover column semantics and it would be cleaner if unified data table exposed a hook for "empty columns fallback" , I didn't find a less invasive way without a larger refactor. Open to revisiting whether users should be able to explicitly choose Summary while a setting exists, but that would be a separate product decision from this PR.
💛 Build succeeded, but was flaky
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
…c#275365) Closes elastic#274632 ## Summary Adds an advanced setting (`observability:apmTraceLogsDefaultColumns`) so APM admins can configure default columns for the **Trace samples → Logs** tab. **Column resolution priority:** 1. URL `logsColumns` param — when present with custom (non-Summary) columns 2. Advanced setting — when non-empty 3. Discover default — `@timestamp` + Summary (`_source`) **Example:** setting `["message"]` shows `@timestamp` + `message` instead of Summary. ### Changes - Register `observability:apmTraceLogsDefaultColumns` advanced setting (technical preview, array of field names, default `[]`) - Surface in **APM → Settings → General settings** - Apply setting in trace samples Logs tab via `getTraceLogsColumns()` merge helper - Read setting via `settings.client` with live subscription (no page reload after save) - URL sync: do not persist Discover Summary/`_source` defaults to `logsColumns` - Add optional `resolveColumnsOnChange` to `@kbn/saved-search-component` (APM-only) to prevent a Summary flash when users remove all columns and a setting is configured ### How it works (confirmed with product) URL session state takes precedence over the admin setting when `logsColumns` contains custom columns. This is intentional: - **URL wins over setting:** If a user customizes columns in the table, those columns are written to the URL and override the admin setting for that session. - **Clearing the setting does not clear URL overrides:** An admin clearing the setting does not reset users who already have `logsColumns` in the URL. Users must reset the table or navigate without the param. - **Setting changes are ignored while URL overrides exist:** If a user has session columns in the URL, admin setting changes do not apply until the URL override is removed. - **Summary not available when a setting is configured:** Removing all columns falls back to the setting columns, not Summary. Users cannot deliberately switch to Summary while the admin setting has values. ### Out of scope (unchanged) - Per-service / per-environment scoping - Changing the global Discover default for all users - Custom field-picker UI for the setting ## Test plan - [ ] Set `message` in APM General settings → Logs tab shows `@timestamp` + `message` - [ ] Save setting → Logs tab updates without page refresh - [ ] Add a column via the table → URL updates, both columns visible - [ ] Remove all columns with setting configured → returns to setting columns - [ ] Empty setting, no URL override → Summary default (`@timestamp` + Summary) - [ ] URL with custom `logsColumns` → URL columns win over setting - [ ] Clear admin setting while URL has columns → URL columns still shown - [ ] Other `LazySavedSearchComponent` consumers (Fleet, Infra, logs-overview) unaffected — `resolveColumnsOnChange` is opt-in and only passed from APM --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
6 participants
Closes #274632
Summary
Adds an advanced setting (
observability:apmTraceLogsDefaultColumns) so APM admins can configure default columns for the Trace samples → Logs tab.Column resolution priority:
logsColumnsparam — when present with custom (non-Summary) columns@timestamp+ Summary (_source)Example: setting
["message"]shows@timestamp+messageinstead of Summary.Changes
observability:apmTraceLogsDefaultColumnsadvanced setting (technical preview, array of field names, default[])getTraceLogsColumns()merge helpersettings.clientwith live subscription (no page reload after save)_sourcedefaults tologsColumnsresolveColumnsOnChangeto@kbn/saved-search-component(APM-only) to prevent a Summary flash when users remove all columns and a setting is configuredHow it works (confirmed with product)
URL session state takes precedence over the admin setting when
logsColumnscontains custom columns. This is intentional:logsColumnsin the URL. Users must reset the table or navigate without the param.Out of scope (unchanged)
Test plan
messagein APM General settings → Logs tab shows@timestamp+message@timestamp+ Summary)logsColumns→ URL columns win over settingLazySavedSearchComponentconsumers (Fleet, Infra, logs-overview) unaffected —resolveColumnsOnChangeis opt-in and only passed from APM