Repository navigation
[Agent Builder] Discover profiles in Agent Builder - #288514
alvarezmelissa87 wants to merge 31 commits into
Conversation
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
|
Thanks @alvarezmelissa87, let me summon @gvnmagni here as the AB expert |
5cf967f to
55cc658
Compare
|
I think we have to tweak a couple of things here, given the constraints of current Chat area (max-width: 768px) we can't expect for that to be wider but there are a few things that we can do. First, if possible I would force table lines to be shorter, in terms of height. We will then need to fix those button/controls at the top and place them in the same line, probably moving the timepicker to the left as it happens for other charts (see image as reference) Last thing, as an open question that I'll bring to the team, an "Expand" button would be very helpful, I'll see how this conflict with the classic logic of opening attachments in flyouts. Let's talk about all this though, these are just very early comments Example of chart with time controls on the left: |
yes ++
Expand as in it opens in full screen? I wonder here if we should be opening the grid in a flyout, but yeah I agree the current size is too small @alvarezmelissa87 can we add the ability to scroll horizontally? Tomasz did something similar on eui tables, maybe we can reuse that? |
|
👋 can we open a ticket in https://github.com/elastic/docs-content-internal/issues to ensure we get documentation for this new feature, along with any key info that would help writers? thanks! |
| attachment_id: z.preprocess( | ||
| (value) => (typeof value === 'string' ? normalizeAttachmentId(value) : value), |
There was a problem hiding this comment.
q: Do we need all this normalization logic? Is the agent struggling with attachment_id?
There was a problem hiding this comment.
Yes. It keeps sending things that aren't real IDs, like ., {attachment_id}, discover-session, or screen-context, instead of leaving attachment_id off. If we reject those, it just retries the same call until it runs out of tool calls. Ignoring the ones we know are fake lets it create the session, or update the only one, on the first try. If it sends an ID we don't recognize, we still reject that.
I also took those examples out of the tool description and the skill. It was copying the values we told it not to use. The check is still there for when it sends one anyway.
Let me know if that makes sense.
|
|
||
| Do not paste rows, tab JSON, or vis_context into the conversation. This skill does not execute ES|QL; the Discover table in chat runs the query. | ||
| `, | ||
| getRegistryTools: () => TOOL_IDS, |
There was a problem hiding this comment.
wdyt about making it inline tool?
There was a problem hiding this comment.
Thanks for taking a look, @rbrtj 🙏
The reason it's a registry tool, is thatcreate_discover_session isn’t unique to this skill. discover-data-analysis also uses it - the in-Discover “analyze my data” flow. After it runs aggregations, if the user asks to see the documents it should open this same live table instead of a Lens data_table. Visualization routing points at the same tool for document tables too.
If we made it inline, it would only show up after this skill is loaded, so Discover analysis could finish and then have no way to render the table unless we copied the tool.
|
|
||
| const TOOL_IDS = [platformCoreTools.generateEsql, platformCoreTools.createDiscoverSession]; | ||
|
|
||
| export const discoverSessionSkill = defineSkillType({ |
There was a problem hiding this comment.
up to you, I guess, but we could mark it as experimental so it is registered only when agentBuilder:experimentalFeatures is ON, just so it can be tested before exposing it by default.
There was a problem hiding this comment.
Good call, @rbrtj. I marked discover-session as experimental, so it only shows up when agentBuilder:experimentalFeatures is on. The tool stays available because discover-data-analysis uses it to open the same table.
| ]; | ||
|
|
||
| const discoverDataAnalysisSkill = defineSkillType({ | ||
| export const discoverDataAnalysisSkill = defineSkillType({ |
There was a problem hiding this comment.
I'm wondering if we need a separate discover session skill? Do you see them running independently or could it be all bundled into a single discoverDataAnalysis skill?
There was a problem hiding this comment.
Good question, @rbrtj. They are separate because they don’t really run the same way. discover-session is the chat path - “show me these logs as a table.” From what I can tell,discover-data-analysis is when you’re already in Discover and you want it to analyze the current query (aggs, chart, drill-downs).
If we bundled the table flow into data-analysis, something like “show me error logs as a document table” would pick up the whole analysis - run a bunch of STATS queries, always draw a chart, dump the overview / drill-down sections. I don't think that's what we want for a simple table.
They share create_discover_session because analysis sometimes needs to open that same table as a follow-up. Sharing the tool is fine but I think it's best to keep the skills separate.
Let me know if that makes sense - happy to change if it doesn't. cc @alexmarhaba to confirm this is still the goal for the discover-session skill.
There was a problem hiding this comment.
I agree I think the current approach is best for now. We can always consolidate later if needed.
There was a problem hiding this comment.
yeah sounds reasonable, lets keep it as is then 👍
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
@alexmarhaba - thanks for taking a look! 🙏 It already supports horizontal scroll so we should be good there! |
… ES|QL from the AST, and keep the empty-result toolbar from clipping no results view
|
Opened a docs request with the writer-facing details: https://github.com/elastic/docs-content-internal/issues/1858 cc @leemthompo |
…listing placeholder attachment ids that the model copies.
florent-leborgne
left a comment
There was a problem hiding this comment.
OAS changes look unrelated but copy LGTM
API Contract Breaking ChangesThe following breaking change(s) were detected across the public OpenAPI surface, grouped by stability tier. Stable and Technical Preview changes fail the check and should be resolved; Experimental changes are informational. Experimental — informational, not blocking merge (2)Experimental APIs are allowed to introduce breaking changes. These are listed for visibility only and do not fail this check.
What to do
See the |
| label: i18n.translate('discover.agentBuilder.openInDiscoverButtonLabel', { | ||
| defaultMessage: 'Open in Discover', | ||
| }), | ||
| handler: () => { |
There was a problem hiding this comment.
Severity: medium
Opening the table in Discover ignores column and sort edits made in the inline grid: the locator always receives the original attachment data, while grid changes are held in the embeddable's live state. After adding/removing a column or sorting by a header, 'Open in Discover' shows the attachment's original layout/sort instead of the table the user was viewing.
onSaveToDashboard above already obtains embeddableApi.getSerializedStateByValue() to capture live table edits, whereas this handler calls getDiscoverSessionLocatorParams({ data, ... }), which reads only tab.column_order and tab.sort from the attachment.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| showQueryInput: false, | ||
| showFilterBar: false, | ||
| showQueryMenu: false, | ||
| showDatePicker: true, | ||
| showSubmitButton: false, | ||
| disableQueryLanguageSwitcher: true, |
There was a problem hiding this comment.
Severity: medium
The local time picker cannot commit a manually entered/absolute range when the legacy EUI date picker is active. In that picker, non-quick changes call onQueryChange, not onQuerySubmit; this hook only handles onQuerySubmit, and it hides the submit button, so the table and Open in Discover keep the old range despite the changed picker display.
QueryBarTopRow.onTimeChange dispatches non-quick selections to propsOnChange, and SearchBarUI.onQueryBarChange only updates its draft state. The inline test mocks SearchBar with a button that invokes onQuerySubmit directly, so it misses this path. Handle the picker change/commit path or provide a submit control for the legacy picker.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
| profileColumns: defaultState.columns, | ||
| fallbackColumns: defaultState.columns === undefined ? [] : defaultColumns, | ||
| dataView, | ||
| esqlQueryColumns, | ||
| }); | ||
|
|
||
| if (columns.length) { |
There was a problem hiding this comment.
Severity: medium
Resetting columns for a profile without getDefaultAppState().columns now discards the configured discover:defaultColumns entirely. Previously those defaults were validated and applied; now getPostFetchState returns no column update, so switching to a profile without column defaults can leave the previous profile's columns in Discover instead of restoring the user's configured defaults.
discover_data_state_container.ts passes uiSettings.get(DEFAULT_COLUMNS_SETTING, []) as defaultColumns during post-fetch profile resets, but this branch replaces it with [] precisely when defaultState.columns is undefined.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
… its current layout and time range.
| const mappedState = useMemo(() => toSearchEmbeddableByValueState(data), [data]); | ||
| const seedTimeRange = getDiscoverSessionSeedTimeRange({ | ||
| mappedTimeRange: mappedState.time_range, | ||
| screenContextTimeRange, | ||
| }); | ||
| const { searchBarProps, effectiveTimeRange } = useDiscoverSessionUnifiedSearch({ | ||
| timeRange: seedTimeRange, | ||
| }); |
There was a problem hiding this comment.
Severity: medium
A time range selected in the inline picker is lost on the next chat response that renders an updated version of the session. The picker updates only this component's local state and embeddable API; it never updates the attachment. If the user selects 15 minutes and then asks a follow-up that changes only the query, the tool preserves the attachment's original time range, and the new inline instance initializes from that old range (for example, 24 hours), silently broadening the results. Preserve the local selection across follow-up renders or synchronize it with the session attachment, and test a fresh mount after a time change and query update.
useDiscoverSessionUnifiedSearch initializes committedTimeRange from this seed, while picker changes only call setCommittedTimeRange. create_discover_session_tool retains the existing time_range when an update omits it; the current follow-up test rerenders the same component instead of mounting a new chat reply.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
There was a problem hiding this comment.
This is expected. The time picker only applies to the current card and doesn't update the attachment. A follow-up creates a new version, so that card starts from the stored range. Saving the selection would mean writing it back to the attachment, which is more than we want to take on here.
| - The user wants raw documents, events, search hits, or a Discover-style document table. Use the discover-session skill and \`${ | ||
| platformCoreTools.createDiscoverSession | ||
| }\` instead. Do **not** use chartType \`"data_table"\` as a substitute for a document table. | ||
| - The user first needs broad data discovery and exploration across unknown sources. |
There was a problem hiding this comment.
Severity: medium
The visualization skill now routes every raw-document request to the discover-session skill and forbids its previous Lens table fallback, even when Agent Builder experimental features are off. In that default mode discover-session is unavailable (experimental: true), and create_discover_session is exposed via that skill or the in-Discover analysis skill, not this visualization skill; a normal chat asking to show raw documents can therefore be directed to a skill/tool it cannot use instead of receiving a table. Make this guidance conditional on availability or retain a supported fallback for users without the experiment.
The skill definition for discover-session sets experimental: true, whereas this visualization skill is not experimental and its tool list does not include createDiscoverSession.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
There was a problem hiding this comment.
Updated to only route to the skill when it's available in 0cbd618
| <div | ||
| css={css` | ||
| height: ${INLINE_TABLE_HEIGHT}px; | ||
| overflow: hidden; | ||
| `} | ||
| > | ||
| <EmbeddableRenderer<SearchEmbeddablePanelApiState, SearchEmbeddableApi> | ||
| key={embeddableKey} | ||
| maybeId={undefined} | ||
| type={SEARCH_EMBEDDABLE_TYPE} | ||
| getParentApi={() => parentApi} | ||
| onApiAvailable={setEmbeddableApi} | ||
| hidePanelChrome |
There was a problem hiding this comment.
Severity: medium
A failing ES|QL request leaves the inline Discover attachment blank instead of showing an error or allowing the user to adjust the local time range. The search embeddable's initializeFetch sets searchError on fetch failure, but its factory renders null for that error outside inline-edit mode; this new chat host supplies no error UI or time picker outside the embeddable. A syntactically valid query against an unavailable index or an Elasticsearch error therefore makes the entire table disappear without an explanation. Provide an error state in this host (or opt the embeddable into an error presentation), with a failing-query regression test.
initialize_fetch.ts catches fetch errors and calls setSearchError(next?.error); get_search_embeddable_factory.tsx returns null for searchError unless isInlineEditing is true.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
There was a problem hiding this comment.
This has been updated in 8c26060. The presentation panel already displays the search error, but the time picker disappeared with the grid toolbar. The inline host now keeps the picker available during blocking errors so users can adjust the range and retry. I also added regression coverage
Co-authored-by: Cursor <cursoragent@cursor.com>
| displayStyle: 'inPage', | ||
| query: EMPTY_KUERY_QUERY, |
There was a problem hiding this comment.
Severity: medium
The legacy SuperDatePicker forwards onQueryChange even when its onTimeChange reports isInvalid: true, but this handler commits every draft range as the active range. While entering an incomplete/invalid absolute time, the inline table calls setTimeRange with that invalid range and starts a fetch rather than retaining the last valid range; a failed fetch can leave the table blank. Ignore invalid legacy-picker changes (or commit only after validation) and cover that path in a test.
QueryBarTopRow.onTimeChange sets isDateRangeInvalid but calls propsOnChange(retVal) for every non-quick selection regardless of isInvalid; SearchBarUI.onQueryBarChange forwards it to onQueryChange. The hook's commitTimeRange updates state without checking validity.
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
There was a problem hiding this comment.
This has been updated in 4c7d5de. We now ignore completed-but-invalid ranges from the legacy picker, including inverted and now-to-now ranges, while retaining the last valid range. Regression coverage has also been added. 👍
|
@elasticmachine merge upstream |
TattdCodeMonkey
left a comment
There was a problem hiding this comment.
agent_builder changes LGTM
The legacy unified search picker forwards completed-but-invalid ranges (end before start, or now-to-now) with isInvalid set, which the inline table was committing and passing to setTimeRange. Skip those so the last valid range stays applied. Co-authored-by: Cursor <cursoragent@cursor.com>
A blocking error replaces the grid, and the time picker lived in that grid's toolbar, so a failed query left the attachment with no way to adjust the range. Render the picker above the panel during the error state instead, and drop it from the toolbar slot so only one is mounted. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
| > => { | ||
| return { | ||
| id: platformCoreTools.createDiscoverSession, | ||
| type: ToolType.builtin, |
There was a problem hiding this comment.
Severity: P2 (Medium)
Exclude this attachment-only tool from MCP. Without excludeFromMcp: true, it is exposed to standalone MCP clients even though its handler relies on conversation attachments and returns a <render_attachment> directive. Those clients cannot access or render the created session, so an apparently successful call does not deliver the promised live document table.
The handler uses attachments.getActive(), getAttachmentRecord(), add(), and update(); its success result contains a conversation-scoped attachment ID and render directive. BuiltInToolSpecificConfig.excludeFromMcp excludes such tools from MCP while retaining their availability in Agent Builder chat.
| type: ToolType.builtin, | |
| type: ToolType.builtin, | |
| excludeFromMcp: true, |
Generated by Libra. React with 👍 or 👎 to give feedback on this comment.
There was a problem hiding this comment.
Updated in 91c3ea4.
This was actually needed. This tool creates an Agent Builder attachment and returns a render tag for the inline Discover table. MCP clients have no conversation attachments or chat UI, so the tool can't work there so it's important to exclude them.
excludeFromMcp landed a few days before this PR started, so I missed it on the first pass.
|
Some non-blocking future follow ups for this change we could make if we want to add support for those things: The below are only reachable through the generic attachment API
Other potential follow-ups
|
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
Unknown metric groups@kbn/rspack-optimizer bundle module count
shared async chunk count
shared async chunks total size
shared chunks total size
total optimizer output size
warm start memory
workflow yaml validation
Test Failures
History
|
PhilippeOberti
left a comment
There was a problem hiding this comment.
LGTM for the @elastic/security-threat-hunting team
Summary
Addresses #283734
DiscoverProfilesInAgentBuilderDemo.mov
Adds inline Discover sessions to Agent Builder chat, allowing users to view raw Elasticsearch documents without leaving the conversation.
Previously, requests for documents could result in pasted search results or a Lens data table. This change introduces a dedicated
discover-sessionskill andcreate_discover_sessiontool so Agent Builder can distinguish between:discover-sessionis marked as experimental for now.User experience
Users can ask to see matching documents and receive an interactive Discover table directly in chat.
The table supports:
For example, Observability logs can display log-level badges, severity indicators, the Summary column, and the Log overview document view:
The 'View details' action in the table still shows the detail flyout:
Implementation
This PR:
discover.sessionAgent Builder attachmentHow to test
Note - this skill is experimental so you will need to either add
uiSettings.overrides.agentBuilder:experimentalFeatures: trueto yourkibana.dev.ymlor flip Agent Builder experimental features on in Stack Management:log.levelvalues have colored badges.Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
release_note:breakinglabel should be applied in these situations.release_note:*label is applied per the guidelinesbackport:*labels.Identify risks
Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.