[TSVB] Replaces hardcoded ids with uuid #97423
Merged
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
stratoula
changed the title
|
@timroes @wylieconlon can you think of any reason that we can't replace these hardcoded ids? |
stratoula
added
Feature:TSVB
wylieconlon
reviewed
Apr 19, 2021
| @@ -25,11 +25,11 @@ export const metricsVisDefinition = { | |||
| group: VisGroups.PROMOTED, | |||
| visConfig: { | |||
| defaults: { | |||
| id: '61ca57f0-469d-11e7-af02-69e470af7417', | |||
| id: uuid.v1(), | |||
There was a problem hiding this comment.
We have chosen to use import uuid from 'uuid/v4' in Lens because it's fully random: uuid v1 is not a random ID.
There was a problem hiding this comment.
Yes Wylie, only here we are using hardcoded ids. Everywhere else we are using uuid.v1()
Nice feedback about v4, will use this instead 🙂
stratoula
added
the
Team:Visualizations
|
Pinging @elastic/kibana-app (Team:KibanaApp) |
flash1293
reviewed
Apr 21, 2021
There was a problem hiding this comment.
This PR makes the problem much less severe, but it's still possible to end up with identical ids because uuid.v4() is only called once on Kibana client start. This means if a user creates multiple TSVB visualizations without reloading the page, they will end up with the same ids again.
If they reload the page, it will work as expected.
Leaving it up to you whether that's good enough.
|
Thanx Joe, no it is not, I will change the implementation. thanx for that 🙂 |
stratoula
changed the title
stratoula
commented
May 6, 2021
| @@ -15,7 +15,6 @@ import { EditorController, TSVB_EDITOR_NAME } from './application/editor_control | |||
| import { createMetricsFn } from './metrics_fn'; | |||
| import { metricsVisDefinition } from './metrics_type'; | |||
| import { | |||
| setSavedObjectsClient, | |||
There was a problem hiding this comment.
Irrelevant with this PR but we dont use anywhere this setter/getter
stratoula
commented
May 6, 2021
| @@ -17,10 +17,6 @@ export const [getFieldFormats, setFieldFormats] = createGetterSetter< | |||
| DataPublicPluginStart['fieldFormats'] | |||
| >('FieldFormats'); | |||
|
|
|||
| export const [getSavedObjectsClient, setSavedObjectsClient] = createGetterSetter<SavedObjectsStart>( | |||
stratoula
changed the title
|
@elasticmachine merge upstream |
|
@flash1293 it should be ok now! |
|
@elasticmachine merge upstream |
flash1293
reviewed
May 12, 2021
There was a problem hiding this comment.
I tested this PR the follwing way:
- Create new dashboard
- Add TSVB panel - save without changing anything
- Add second TSVB panel - save again without changing anything
- Hitting refresh
It would expect the requests sent by the panels to be different, but all ids are identical.
|
@flash1293 yes, I know about this case but it seems to be a very edge case. It happens because you don't change anything on the editor, so it initializes with the same id. |
|
@stratoula Hm, I did the following now:
In the request to TSVB, the panel ids are different, but series ids and metric ids are still identical. In the issue linked above it seems like these are the ones this is all about:
I also checked the request sent to ES from TSVB and it does not include the panel id, but series and metric id. panel 1: panel 2: requests: As a quick fix we might be able to add the panel id to the |
|
Thank you Joe! I will try to add the panel id to meta. |
flash1293
approved these changes
May 12, 2021
|
@flash1293 thank you a lot for your help with this issue ❤️ |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / Chrome UI Functional Tests.test/functional/apps/visualize/input_control_vis/chained_controls·ts.visualize app input controls chained controls should filter child control options by parent control valueStandard OutStack TraceMetrics [docs]Async chunks
Page load bundle
Unknown metric groupsReferences to deprecated APIs
History
To update your PR or re-run it, just comment with: |
stratoula
added a commit
to stratoula/kibana
that referenced
this pull request
May 12, 2021
* [TSVB] Replace hardcoded ids with uuid * Use v4 instead * Add id on the model initialization * Change the id in case of new visualization * Add paneld to the meta object Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
stratoula
added a commit
that referenced
this pull request
May 12, 2021
* [TSVB] Replace hardcoded ids with uuid * Use v4 instead * Add id on the model initialization * Change the id in case of new visualization * Add paneld to the meta object Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
This was referenced Aug 16, 2021
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.
Summary
Closes #96670.
It removes the hardcoded ids from TSVB and replaces them with uuid.
I have made some tests and I can't find any regression or any reason that we can't do it.
Example of a TSVB SO: