Skip to content

Comments

🌊 Streams: Use better default field#217478

Merged
flash1293 merged 6 commits intoelastic:mainfrom
flash1293:flash1293/smart-default-state
Apr 15, 2025
Merged

🌊 Streams: Use better default field#217478
flash1293 merged 6 commits intoelastic:mainfrom
flash1293:flash1293/smart-default-state

Conversation

@flash1293
Copy link
Contributor

@flash1293 flash1293 commented Apr 8, 2025

This PR passes the current sample documents to the default form state generation for new processors to pick a good default field.

The logic that's actually employed for dissect and grok is the following:

  • Go through all docs and order string fields occurring by how many values they have
  • Pick the top one from a list of "well known" fields that probably make sense (in case of a tie, go by a the ordering of the well known fields)
  • If no field is found this way, just leave it empty - this still shows the full table and the user can pick the field they care about

Especially for otel this should be helpful.

@flash1293 flash1293 added release_note:skip Skip the PR/issue when compiling release notes Team:obs-onboarding Observability Onboarding Team backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project v9.1.0 v8.19.0 labels Apr 8, 2025
@flash1293
Copy link
Contributor Author

/ci

@flash1293
Copy link
Contributor Author

/ci

@flash1293 flash1293 marked this pull request as ready for review April 8, 2025 13:35
@flash1293 flash1293 requested a review from a team as a code owner April 8, 2025 13:35
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs)

Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but would add a few tests for the detection logic (see below)

@tonyghiani
Copy link
Contributor

@flash1293 a couple of things:

  • WRT computing the default field for each processor, why not keep the static definition of the default forms (switching the default to an empty string) and merge the definition where required on the processor forms? This would keep every piece well decoupled and we would just need to get the default field with a selector, removing any other change done to the existing interface and making the default form static again. In pseudo-code:
// selectors.tsx
export const selectDefaultField = createSelector(
  [(context: SimulationContext) => context.samples],
  (samples) => getDefaultTextField(samples)
);

// then in the components
const defaultFied = useSimulatorSelector((snapshot) => selectDefaultField(snapshot.context));

const formState = { ...getDefaultFormStateByType(type), field: defaultFied };

The changes would be minimal, and we don't need to change anything on the existing previewDocuments selector.

  • Do we need to use the previewDocuments? Why not just the base samples?

@flash1293
Copy link
Contributor Author

@tonyghiani I thought about this, but a good suggestion depends on the processor (e.g. for the date processor we wouldn't suggest the same fields). It's not even a given that all processors have a field to operate on.

It could still be kept in the component, but it seems like it doesn't belong there.

Do we need to use the previewDocuments? Why not just the base samples?

Not sure, that would work too. I thought it's always nice to stay consistent and base all decisions on what the user currently sees so they can understand where things are coming from.

@tonyghiani
Copy link
Contributor

I thought about this, but a good suggestion depends on the processor (e.g. for the date processor we wouldn't suggest the same fields). It's not even a given that all processors have a field to operate on.

Agree, the approach I was proposing would still work well since we have the processor type in scope

// selectors.tsx
export const selectDefaultField = createSelector(
  [
    (context: SimulationContext) => context.samples,
    (type: ProcessorType) => type
  ],
  (samples, type) => getDefaultTextField(samples, type)
);

// then in the components
const defaultFied = useSimulatorSelector((snapshot) => selectDefaultField(snapshot.context, type));

const formState = { ...getDefaultFormStateByType(type), field: defaultFied };

@flash1293
Copy link
Contributor Author

Right, I see - no strong opinion, I can change it like that

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
streamsApp 405.7KB 406.6KB +979.0B

History

@flash1293 flash1293 merged commit e6cdba6 into elastic:main Apr 15, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/14469423743

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 15, 2025
This PR passes the current sample documents to the default form state
generation for new processors to pick a good default field.

The logic that's actually employed for `dissect` and `grok` is the
following:
* Go through all docs and order string fields occurring by how many
values they have
* Pick the top one from a list of "well known" fields that probably make
sense (in case of a tie, go by a the ordering of the well known fields)
* If no field is found this way, just leave it empty - this still shows
the full table and the user can pick the field they care about

Especially for otel this should be helpful.

(cherry picked from commit e6cdba6)
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Could not create pull request: Unexpected end of JSON input

Manual backport

To create the backport manually run:

node scripts/backport --pr 217478

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 17, 2025
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217478 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217478 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217478 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 217478 locally

@flash1293
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

flash1293 added a commit to flash1293/kibana that referenced this pull request Apr 23, 2025
This PR passes the current sample documents to the default form state
generation for new processors to pick a good default field.

The logic that's actually employed for `dissect` and `grok` is the
following:
* Go through all docs and order string fields occurring by how many
values they have
* Pick the top one from a list of "well known" fields that probably make
sense (in case of a tie, go by a the ordering of the well known fields)
* If no field is found this way, just leave it empty - this still shows
the full table and the user can pick the field they care about

Especially for otel this should be helpful.

(cherry picked from commit e6cdba6)
flash1293 added a commit that referenced this pull request Apr 23, 2025
# Backport

This will backport the following commits from `main` to `8.19`:
- [🌊 Streams: Use better default field
(#217478)](#217478)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Joe
Reuter","email":"johannes.reuter@elastic.co"},"sourceCommit":{"committedDate":"2025-04-15T12:29:09Z","message":"🌊
Streams: Use better default field (#217478)\n\nThis PR passes the
current sample documents to the default form state\ngeneration for new
processors to pick a good default field.\n\nThe logic that's actually
employed for `dissect` and `grok` is the\nfollowing:\n* Go through all
docs and order string fields occurring by how many\nvalues they have\n*
Pick the top one from a list of \"well known\" fields that probably
make\nsense (in case of a tie, go by a the ordering of the well known
fields)\n* If no field is found this way, just leave it empty - this
still shows\nthe full table and the user can pick the field they care
about\n\nEspecially for otel this should be
helpful.","sha":"e6cdba65ed8263cf0ffcd2ba6b14d7caa579432b","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport
missing","Team:obs-ux-logs","backport:version","Feature:Streams","v9.1.0","v8.19.0"],"title":"🌊
Streams: Use better default
field","number":217478,"url":"https://github.com/elastic/kibana/pull/217478","mergeCommit":{"message":"🌊
Streams: Use better default field (#217478)\n\nThis PR passes the
current sample documents to the default form state\ngeneration for new
processors to pick a good default field.\n\nThe logic that's actually
employed for `dissect` and `grok` is the\nfollowing:\n* Go through all
docs and order string fields occurring by how many\nvalues they have\n*
Pick the top one from a list of \"well known\" fields that probably
make\nsense (in case of a tie, go by a the ordering of the well known
fields)\n* If no field is found this way, just leave it empty - this
still shows\nthe full table and the user can pick the field they care
about\n\nEspecially for otel this should be
helpful.","sha":"e6cdba65ed8263cf0ffcd2ba6b14d7caa579432b"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/217478","number":217478,"mergeCommit":{"message":"🌊
Streams: Use better default field (#217478)\n\nThis PR passes the
current sample documents to the default form state\ngeneration for new
processors to pick a good default field.\n\nThe logic that's actually
employed for `dissect` and `grok` is the\nfollowing:\n* Go through all
docs and order string fields occurring by how many\nvalues they have\n*
Pick the top one from a list of \"well known\" fields that probably
make\nsense (in case of a tie, go by a the ordering of the well known
fields)\n* If no field is found this way, just leave it empty - this
still shows\nthe full table and the user can pick the field they care
about\n\nEspecially for otel this should be
helpful.","sha":"e6cdba65ed8263cf0ffcd2ba6b14d7caa579432b"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:version Backport to applied version labels Feature:Streams This is the label for the Streams Project release_note:skip Skip the PR/issue when compiling release notes Team:obs-onboarding Observability Onboarding Team v8.19.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants