-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
KQL support in filter ratio in TSVB #75033
KQL support in filter ratio in TSVB #75033
Conversation
src/plugins/vis_type_timeseries/public/application/components/aggs/filter_ratio.js
Outdated
Show resolved
Hide resolved
src/plugins/vis_type_timeseries/server/lib/vis_data/request_processors/series/filter_ratios.js
Outdated
Show resolved
Hide resolved
...gins/vis_type_timeseries/server/lib/vis_data/request_processors/series/filter_ratios.test.js
Outdated
Show resolved
Hide resolved
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.
see comments above
src/plugins/vis_type_timeseries/public/application/components/aggs/filter_ratio.js
Outdated
Show resolved
Hide resolved
src/plugins/vis_type_timeseries/public/application/components/aggs/filter_ratio.js
Outdated
Show resolved
Hide resolved
src/plugins/visualizations/server/saved_objects/visualization_migrations.ts
Outdated
Show resolved
Hide resolved
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 LGTM! Added some nit's
Do we plan to fix this here? |
// Let it go, the data is invalid and we'll leave it as is | ||
} | ||
if (visState && visState.type === 'metrics') { | ||
const series: any[] = get(visState, 'params.series') || []; |
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.
I assume we can't avoid these anys here, right? 😢
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.
If I'm not wrong it's SeriesItemsSchema
from src/plugins/vis_type_timeseries/common/types.ts. But not sure that SeriesItemsSchema
was a good name for that
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.
Oh... sorry, it's a migration script. I'm ok to see any
here cause otherwise we should create a new type for each migration.
schema.object({ | ||
language: schema.string(), | ||
query: schema.string(), | ||
}) |
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.
There is a queryObject
variable to replace
<EuiFieldText onChange={handleTextChange('numerator')} value={model.numerator} /> | ||
<QueryBarWrapper | ||
query={model.numerator} | ||
onChange={(query) => handleQueryChange('numerator', query)} |
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.
It is used to wrap callbacks into useCallback
to avoid extra re-rendering.
const handleNumeratorQueryChange = useCallback((query) => handleChange({ numerator: query }), [
handleChange,
]);
I know this won't give a lot of effect, since the handleChange
is always newly created, but you could also wrap it in useMemo
. In such a case we'll make a short step into performance optimization =)
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.
LGTM
Let's resolve the scrollbar issue in #73250
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.
LGTM, agree with @sulemanof 🍪
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
const indexPattern = | ||
(series.override_index_pattern && series.series_index_pattern) || panel.index_pattern; | ||
|
||
const defaults = { | ||
numerator: '*', | ||
denominator: '*', | ||
numerator: { query: '', language: getDefaultQueryLanguage() }, |
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.
Is it possible to use data.query.getDefaultQuery()
instead?
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.
Tested and works.
Added one small request.
Otherwise LGTM
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
* KQL support in filter ratio in TSVB Closes elastic#67503 * Fix filter_ratio and filter_ratios tests * fix JEST * Refactor some code in filter_ratio, filter_ratios, filter_ratios.test * Edit query value in filter_ratio and filter_ratios.test * Refacor some code in filter_ratio.js and visualization_migrations.ts * Remove duplications in vis_schema and refactor filter_ratio * Refactor filter_ratio.js * Update default query with getDefaultQuery() * Fix filter_ratio and histogram_support tests Co-authored-by: Alexey Antonov <alexwizp@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* KQL support in filter ratio in TSVB Closes #67503 * Fix filter_ratio and filter_ratios tests * fix JEST * Refactor some code in filter_ratio, filter_ratios, filter_ratios.test * Edit query value in filter_ratio and filter_ratios.test * Refacor some code in filter_ratio.js and visualization_migrations.ts * Remove duplications in vis_schema and refactor filter_ratio * Refactor filter_ratio.js * Update default query with getDefaultQuery() * Fix filter_ratio and histogram_support tests Co-authored-by: Alexey Antonov <alexwizp@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Alexey Antonov <alexwizp@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Closes #67503
Summary
Enabled KQL support in filter ratio in TSVB
Checklist
Delete any items that are not applicable to this PR.
For maintainers