-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
TSDB support for Lens, TSVB and Timelion #139020
Conversation
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
Since the results are partial instead of the application, would it make sense to rename How about this for the tooltip:
|
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
This is different than the “incompatible” dot, as it’s possible to use this function with the current field, but it might produce partial data. Requiring the user to hover over the dot to learn about this seems too subtle to me, but if you and @ghudgins think it would be the way to go I’m happy with that too. What about a different kind of indicator for this? |
I wrote my comment before you added the last sentence - agreed with that 😁 no concerns with an indicator per function in a unified list but it should be different than the existing one. |
I get this error when I run These are the contents of my I tried installing Eclipse and using it to open Do you know how to solve this @flash1293 ? |
Unfortunately I don't, I didn't run into this error. @spalger do you know what's going on here? #139020 (comment) Edit: Checking the code of
This is weird, the comment says it is only checking modified files but then it tries to iterate over
. Is this working as intended? @andrewctate To make it work temporarily I have two ideas what to try:
|
I have very limited experience with |
Cool. EUI happens to have a |
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 tested all three apps and went through the code. LGTM—just had a few questions.
1.
"unsafe" functions that won't work
Why do we allow these to be selected at all?
2.
I verified that both TSVB and Timelion switch their requests to use UTC as soon as they're looking at rolled-up data. However, in my case, they both attached fixed_interval regardless of whether or not they were dealing with rolled up data. Could have been my visualization configuration, but thought it was worth mentioning since I didn't figure out how to make them use calendar interval for non-rolled-up data.
3.
Am I correct in understanding that we are ultimately relying on the field caps API to report a list of possible fixed intervals for a rolled-up field?
All my other comments will be addressed by your follow-up list 👍
!referencedField || | ||
!referencedField.softRestrictions || | ||
!referencedField.softRestrictions[navItem.id!] |
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.
!referencedField || | |
!referencedField.softRestrictions || | |
!referencedField.softRestrictions[navItem.id!] | |
!referencedField?.softRestrictions?.[navItem.id!] |
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.
Why do we allow these to be selected at all?
Because the time range can change and they can suddenly work. The time range dictates if the indexes that are returned are downsampled or not
x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/percentile.tsx
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/operations/operations.ts
Outdated
Show resolved
Hide resolved
@@ -62,6 +62,19 @@ export function getSortScoreByPriority( | |||
return (b.priority || Number.NEGATIVE_INFINITY) - (a.priority || Number.NEGATIVE_INFINITY); | |||
} | |||
|
|||
export const getSortScoreByPriorityForField = |
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.
does a unit test make sense here?
That's correct, but we don't leverage the specific value somewhere, it's just about whether there is an index which is rolled up in a certain way - if it is, we can still use other fixed intervals without error (even if smaller than the rollup interval), but we can't safely apply calendar intervals anymore. |
Thanks for the review, addressed all comments.
As Graham noted, this is a "soft" restriction because in 99% of the cases there is also recent non-rolled up data for which it makes sense to apply these functions. So we allow users to pick it and live with the warnings. Note that in case of the sample data as described above the rolled-up and regular data is in the same time range but that's rather unusual. In most cases the rolled up data will be in another time range (e.g. last week won't be rolled up, but the month before that will), so the user won't actually get a warning as long as looking at the last week.
As long as the selected data view includes at least one index that's rolled up that's the expected behavior - it's difficult but possible to detect this situation on a per-query basis and adjust the interval based on this information, but we decided to not go with this as it will be extra confusing when suddenly crossing the boundary (for example looking at the last 5 days will give you calendar intervals in the current time zone, changing the time range to 8 days will suddenly give you fixed intervals in UTC). Data views which don't include rolled up data at all should not be affected (not sure which case you ran into - the described behavior is what I'm seeing locally) This is how the function list looks now (I like it, what do you think @MichaelMarcialis ?): Note that the "incompatible" dot will never show up together with the partial icon - if a field is incompatible with a function the current field is not transferred over so we do not know whether it will be partially applicable or not (as this depends on the choice the user didn't make yet) |
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.
Thanks for making that change to the quick function list, @flash1293! That looks much better to my eyes. I'm leaving a few comments below for your review, but nothing earth-shattering. Assuming those get addressed, I'm approving now so I don't hold you up further.
- I noticed that the "Search field names" input above the field list has a double border (CSS
box-shadow
in reality) effect happening in this PR that doesn't seem to appear in themain
branch. Any chance to fix it so this artificial thickening of the input border doesn't happen?
- Since you mentioned that the quick functions' incompatible
dot
icons can't appear simultaneously with the newly implementedpartial
icon, would you be willing to try coloring thepartial
icon in the same yellow color we use for the incompatibledot
icons? Assuming it doesn't look awful, that might help to both visually offset it from the text color and be consistent with the notion that it's a warning.
x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
dots look good. sorry for the late 👀 |
* lens field filter and default behavior and utc/fixed interval in tsvb and timelion * fix problems * Update esaggs.test.ts * review comments * fix i18n * review comments
Fixes #131517
Fixes #131518
Fixes #136403
Fixes #136404
This PR is implementing the first parts of supporting TSDB in Lens, TSVB and Timelion.
How to test
sample-01
for testing TSDB without rollups and a data viewsample-01,sample-01-rollup
for testing TSDB with rollupsLens
TSVB
If the current data view is hitting rolled up data, date histogram aggregations are always using fixed_interval instead of calendar_interval and UTC instead of the configured timezone.
This can be checked via the inspector.
It should not influence TSVB charts which are executed on index patterns not hitting rolled up timeseries indices.
Timelion
Like TSVB, if the current data view is hitting rolled up data, date histogram aggregations are always using fixed_interval instead of calendar_interval and UTC instead of the configured timezone.
This can't be checked via inspector as Timelion isn't reporting executed requests - the easiest way to test is to add a
console.log
for the sent search inkibana/src/plugins/vis_types/timelion/server/series_functions/es/index.js
Line 131 in 1681ebe
Split out intentionally