-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SLO Form] Use saved Data view id , handle runtime mappings #176662
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
1130fff
to
c54d750
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall that's a good idea that we had also with Chris a while back.
But I'm a bit concerned with including all runtime fields every time they exists when we create a transform with a DataView. I think we can do two things to reduce the risk on performance:
- Add the runtime fields when the user specifically asks for it
- Add only the runtime fields that are necessary for the different queries
The second option is obviously harder and more cumbersome in terms of surface API that need to changed, but would not require the first option at all.
The first option gives us at least a safety switch, and moves any performance issue into the customer hands, and is very easy to implement.
So if we move forward with this, I'd request for at least the settings option to be included as well (slo.settings can be used).
What do you think?
x-pack/plugins/observability/server/services/slo/transform_generators/transform_generator.ts
Outdated
Show resolved
Hide resolved
...observability/server/services/slo/summary_transform_generator/summary_transform_generator.ts
Outdated
Show resolved
Hide resolved
...k/plugins/observability/server/services/slo/transform_generators/apm_transaction_duration.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/server/services/slo/transform_generators/histogram.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/server/services/slo/transform_generators/metric_custom.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability/server/services/slo/transform_generators/timeslice_metric.ts
Outdated
Show resolved
Hide resolved
Pinging @elastic/obs-ux-management-team (Team:obs-ux-management) |
@kdelemme i have addressed the rest of the comments, let me know if we need further discussion regarding run time mappings |
@elasticmachine merge upstream |
…kibana into form-use-data-view-id
I won't have time to test it locally, but code change LGTM. Chris to take over
It looks like the only indicator that works with a "scripted field" is KQL. They are not showing up in the filters for any of the other indicators. I was also expecting to be able to use them as a field for a "Timeslice Metric" and "Custom Metric" which it doesn't seem to appear in the field drop down. For reference, I created a scripted field on the "Message Processor" data view from |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Canvas Sharable Runtime
Page load bundle
History
To update your PR or re-run it, just comment with: |
|
||
const { isLoading: isDataViewsLoading, data: dataViews = [], refetch } = useFetchDataViews(); | ||
|
||
const { dataViewEditor } = useKibana().services; | ||
|
||
const { dataViewEditor } = useKibana<SloPublicPluginsStart>().services; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
merge with above useKibana()
Users will need to harden their runtime mappings when used in an SLO otherwise the transform won't start: For example:
|
Summary
Fixes #173771
Use saved data view id instead of index pattern where it's available.
Inject runtime mappings from the dataview into transform.
We are not supporting "scripted fields" from the Index Management DataView editor.