Skip to content
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

Warning when TSVB instead of Lens is used #317

Closed
Tracked by #539
ruflin opened this issue Apr 12, 2022 · 12 comments · Fixed by #610
Closed
Tracked by #539

Warning when TSVB instead of Lens is used #317

ruflin opened this issue Apr 12, 2022 · 12 comments · Fixed by #610
Assignees
Labels
Team:Ecosystem Label for the Packages Ecosystem team

Comments

@ruflin
Copy link
Member

ruflin commented Apr 12, 2022

Most of the time series visualisations are built based on TSVB. The long term plan is move these to Lens. Even though Lens does not have all the features yet of TSVB, many simple visualisations can be migrated over. During the validation of a package, it should be checked if TSVB visualisations are used and a warning should be raised.

@jlind23 jlind23 added the Team:Ecosystem Label for the Packages Ecosystem team label Apr 12, 2022
@jsoriano
Copy link
Member

Even though Lens does not have all the features yet of TSVB, many simple visualisations can be migrated over.

I wouldn't like to have warnings on issues I cannot do anything about. Also, migration of complex time series visualizations can be complicated and error prone, and developers may feel tempted to try it after seeing the warning, leading to frustration if not possible at the end.

I think that ideally there should exist some tool to migrate automatically from TSVB to Lens, at least for the simple cases. If not possible to generate the Lens visualizations, maybe at least we can detect what are these simple cases and warn only on them.

Also, TSDB efforts may change the way we build visualizations for time series soon. We probably want to wait till this is available to make a decision on what to do with TSVB visualizations. Otherwise developers may end up migrating visualizations twice in a few versions.

@ruflin
Copy link
Member Author

ruflin commented Apr 12, 2022

AFAIK there is already some tooling in place in the UI to automatically migrate some of the visualisations from Lens to TSVB. @flash1293 will know more here.

The main problem I'm trying to solve here, is that we keep adding TSVB visualisations for many things where Lens has the abilities we need. It would be likely possible to detect which ones can be migrated or not but I'm not sure if the engineering effort for this is overkill. If we can't build a visualisation in TSVB, it should force us to talk to the Lens team. Most of the limitations are well known and soon to be fixed.

The move to Lens should have started quite some time ago but we keep building on TSVB because we are just used to it. We should not couple this to the TSDB effort as it is unrelated. Actually being on Lens when TSDB fully lands will be an advantage.

The reason I brought up the idea around a warning is to push every developer of packages to using Lens and raise issues if it is not possible. Maybe there are better solutions to this.

@flash1293
Copy link
Contributor

flash1293 commented Apr 12, 2022

That's right, there is a "convert to Lens" button in the TSVB editor in 8.2 (doesn't work for all cases yet, we are going to improve it over time). We can definitely look into making this logic accessible from outside of the UI for automated conversion at some point (e.g. via API on the Kibana server). However for newly created dashboards / visualizations, the engineers working on them should be aware of all of the tools and best practices to follow given the current "state of the art". I'm going to prepare this within the next two weeks, maybe we can revisit this after I put it together.

@jsoriano
Copy link
Member

AFAIK there is already some tooling in place in the UI to automatically migrate some of the visualisations from Lens to TSVB.

That's right, there is a "convert to Lens" button in the TSVB editor in 8.2 (doesn't work for all cases yet, we are going to improve it over time). We can definitely look into making this logic accessible from outside of the UI for automated conversion at some point (e.g. via API on the Kibana server)

Oh, something like this would be great. This way we could do a more reliable check, and warn developers only in cases where a migration is possible.

The move to Lens should have started quite some time ago but we keep building on TSVB because we are just used to it.

However for newly created dashboards / visualizations, the engineers working on them should be aware of all of the tools and best practices to follow given the current "state of the art". I'm going to prepare this within the next two weeks, maybe we can revisit this after I put it together.

Agree, maybe we should start with a communication effort?

We should not couple this to the TSDB effort as it is unrelated. Actually being on Lens when TSDB fully lands will be an advantage.

I don't think this is completely unrelated. For example the only way I know today to visualize counters with multiple time series is with TSVB (with this approach). TSDB is likely going to help there, and may change how queries and visualizations for this and other of our use cases are done.
Maybe I am outdated on this, but this brings me back to the communication effort that we may need to increase use of Lens between package developers.

Don't get me wrong, I am totally +1 with increasing the use of Lens, and happy to see that it helps more and more in our use cases. But I would be wary about adding noise to package development.

@flash1293
Copy link
Contributor

I don't think this is completely unrelated. For example the only way I know today to visualize counters with multiple time series is with TSVB

In case of summing up the multiple counters into a single value it's still true (but we are working on this feature for Lens right now).

However, in terms of TSDB Lens will be the first one to get full support for all of the new functionality (and it's not decided yet how far we will enable the TSVB and agg based editors). I'm going to include this in my document, but I think the best guidance for new dashboards/visualizations would be:

  • Use by-value, not by-reference (except for a really good reason not to)
  • Use Lens if it supports all of the necessary functionality already, otherwise fall back to TSVB

IMHO we can try go with the communication effort first (I will keep you posted on that one), see how well it works and add automation like warnings as soon as patterns emerge.

@ruflin
Copy link
Member Author

ruflin commented Apr 13, 2022

There was also an offline discussion on what we could do to notify devs to use Lens instead of TSVB.

One idea is to use a label. By default, CI would complain if a TSVB visualisation is added. Devs then have to add a label use_tsvb to skip the check and ideally provide a quick note on why TSVB has to be used. This allows devs to still used TSVB if needed and would allow us to ping the Lens team in all these scenarios to see what can be done about this TSVB usage long term.

The data driven approach was also discussed. Thanks to @flash1293 we already have some stats on usage, we could use this even more and communicate it to the team regurarely.

The communication effort is needed in any way and we need to repeat it frequently. I personally would quite like to add the label check for TSVB afterwards as it makes sure everyone is constantly aware. But this would likely only be in the integrations repository.

@akshay-saraswat
Copy link

We can definitely look into making this logic accessible from outside of the UI for automated conversion at some point (e.g. via API on the Kibana server)

@flash1293 please confirm that this API is available for us to use. If yes, we will go for the following solution:

API Validation

  • Test the new package versions using Kibana API by checking if a TSVB to lens visualization migration for the given package is possible or not.
  • Warn the package author if the validation mentioned above is feasible.

If the API is not available, let's work on the idea proposed by @ruflin in the comment above:

Flag and reason

  • Check if the given package has any TSVB visualizations and throw an error.
  • Package author would have the option to pass a flag skip-tsvb-check and this flag would require a github issue reference for the missing capability that is required for the migration.
  • (nice-to-have) Post the package name dependency as a comment automatically in the given github issue.

@flash1293
Copy link
Contributor

@akshay-saraswat We don't have that API at the moment and no immediate plans to build it - it's a viable option for later phases but we didn't decide on it yet. So Nicolas idea seems more practical in the short term

@akshay-saraswat
Copy link

Ok. We'll go ahead with "Flag and reason" solution then.

@jsoriano jsoriano removed their assignment Nov 8, 2022
@agithomas
Copy link
Contributor

Can we have this taken up soon? Multi-sprint iterations were taken up to have almost all visualisation migrated to Lens.

Can we have the warnings appear both when running elastic-package check & elastic-package build.

Also, suggest representing the errors in a tabular manner to categorize various types of errors to improve readability.

Reference

image

@jsoriano
Copy link
Member

jsoriano commented Aug 24, 2023

Given that almost everything has been migrated to lens, maybe we can directly fail when TSVB is used. Adding this to Package Spec v3 (#539) so we can make the breaking change.

@drewdaemon
Copy link
Contributor

I'm excited to discover this discussion, because I was independently thinking along the very same lines.

I would suggest broadening the scope of this from a TSVB check to simply "legacy content" since we should be migrating away from several things

  • TSVB
  • Aggs-based visualizations
  • Legacy input controls

We (the visualizations team) have code we use to track legacy Kibana content usage in the integrations. I have made progress toward converting this to golang (see here) and I think we could reuse this in elastic-package, or perhaps even publish the logic as a dependency for both our tracking scripts and package-spec to make sure we stay aligned around the definition of "legacy content" moving forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Ecosystem Label for the Packages Ecosystem team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants