[TSVB] Chart with override index pattern and last_value are migrated with entire_time_range from 7.13+ #115041
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
VladLasitsa
added
Feature:TSVB
|
@elasticmachine merge upstream |
stratoula
added
release_note:fix
and removed
release_note:skip
|
Pinging @elastic/kibana-vis-editors (Team:VisEditors) |
stratoula
approved these changes
Oct 18, 2021
There was a problem hiding this comment.
Code LGTM, I tested it locally by migrating 3 SOs from 7.11 to the current branch:
- One metric with
override_index_patternand last value - One metric with
override_index_patternand entire time range - One TopN with two layers, one with
override_index_patternand last value and one withoverride_index_patternand entire time range
All of them were migrated correctly!
alexwizp
reviewed
Oct 18, 2021
| @@ -79,6 +79,15 @@ export class VisEditor extends Component<TimeseriesEditorProps, TimeseriesEditor | |||
| ? TIME_RANGE_DATA_MODES.LAST_VALUE | |||
| : TIME_RANGE_DATA_MODES.ENTIRE_TIME_RANGE, | |||
| ...this.props.vis.params, | |||
| series: this.props.vis.params.series.map((val) => ({ | |||
There was a problem hiding this comment.
@stratoula @VladLasitsa maybe it should go into migration script?
There was a problem hiding this comment.
This is a good point but we should add a migration in 7.16 about a previous version (7.13) and I think we are trying to avoid it. This seems to me as a not very common case but I would like @flash1293 feedback here!
There was a problem hiding this comment.
Is this within the editor or is it also used on the dashboard?
I think we should do this:
- Make sure this fix also works if the user never opens the editor
- Move it to the migrations if possible - it doesn't matter if a bug introduced in 7.13 is only fixed in 7.16; if we are confident enough to put this logic in the code showing the visualization we can be confident enough putting it into the migration script running for the next version.
There was a problem hiding this comment.
Yes Joe, it works for the dashboard panels but I agree with your point. Let's also add a migration script :)
There was a problem hiding this comment.
@stratoula Could you please describe for which version we should add migration script and in this case we should remove this part of code, right?
There was a problem hiding this comment.
When we create new vis and don't change time range mode, we don't provide it to model
Is there a reason why we are not storing it?
There was a problem hiding this comment.
@flash1293 When we create new tsvb we have only default state of time range mode which provide in model only my part of code. without this code we can see on UI right time range mode but in model we don't have it while we select another mode in combobox.
There was a problem hiding this comment.
No Joe, I guess this also follows the logic "This is the default, do not store it :/". And now we changed the default and this is the reason we have this kind of bugs.
@VladLasitsa I got confused tbh by Alexey's comment about the migration. So if your fix works for all cases, then yes no migration script is needed. It is either a migration script or the fix. If we can't provide a migration script, the fix seems fine to me.
There was a problem hiding this comment.
@flash1293, you don't mind to leave only fix without migration script?
There was a problem hiding this comment.
@VladLasitsa sounds good, your reasoning makes sense to me.
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Saved Object Tagging Functional Tests.x-pack/test/saved_object_tagging/functional/tests/dashboard_integration·ts.saved objects tagging - functional tests dashboard integration editing allows to select tags for an existing dashboardStandard OutStack TraceMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @VladLasitsa |
VladLasitsa
added a commit
to VladLasitsa/kibana
that referenced
this pull request
Oct 21, 2021
…with entire_time_range from 7.13+ (elastic#115041) * Move logic about default time range mode inside index pattern component * Fix lint * Fix types * Cover case with override_index_pattern Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
VladLasitsa
added a commit
that referenced
this pull request
Oct 21, 2021
…with entire_time_range from 7.13+ (#115041) (#115930) * Move logic about default time range mode inside index pattern component * Fix lint * Fix types * Cover case with override_index_pattern Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
shivindera
pushed a commit
to shivindera/kibana
that referenced
this pull request
Oct 25, 2021
…with entire_time_range from 7.13+ (elastic#115041) * Move logic about default time range mode inside index pattern component * Fix lint * Fix types * Cover case with override_index_pattern Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes: #114984
Summary
Fixes the bug for visualizations before 7.13 which use the
override_index_patternsetting andLast valueas the timerange mode. The bug is that when they are migrated to 7.13+ instances, they are using theentire time rangeinstead of thelast value.