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

chore: use initLog() in utils, to handle use case when LSP server is not loaded #6424

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

wiedld
Copy link
Contributor

@wiedld wiedld commented Dec 20, 2022

Follow up to influxdata/flux-lsp#618

The LSP wasm was panicking due to use of a logger which was not compatible with wasm. The recent LSP version bump included:

  1. a global wasm logger init whenever the LSP server was init.
    • no UI change required, outside of the version bump
  2. the option to manually init the wasm logger, when the LSP utils methods are used without the LSP server being init first
    • this does require calling initLog() in the UI, to handle race condition if LSP server is not loaded --> but LSP utils methods are used
    • setting the global logger >1 time, has no negative impact. (Only resulted in console warning within the LSP tests.)

Impact in UI.

We do have a current, frequent bug in the UI where the dashboard fails after a panic occurs in the LSP wasm. As we did not know the root of the panic (and we didn't have any error logs from the wasm), we focused instead on the cascading nature of the variables tie-in with dashboards.

Recently, we became aware of that fact that our lack of logging from wasm in prod (especially during instances of wasm panic) was directly due to our logging causing the panics. This is a followup PR to ensure that the global logger is always set.

Checklist

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@wiedld wiedld marked this pull request as ready for review December 20, 2022 12:17
@wiedld wiedld requested a review from a team as a code owner December 20, 2022 12:17
@wiedld wiedld requested review from a team and abshierjoel December 20, 2022 12:17
parse as flux_parse,
format_from_js_file as flux_format_from_js_file,
} from '@influxdata/flux-lsp-browser'

initLog()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it possible to call this elsewhere? Side effects in modules like this should be avoided if possible. This function will be called anytime code imports parse or format_from_js_file. Is it possible to move this function call into the parse and format_from_js_file functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the LSP doesn't have a single entry point, @hoorayimhelping and I agreed to leave this code as the best alternative. Doc'ing the tech debt in the LSP, that we need some refactoring to enable the single entry point.

@wiedld wiedld added this pull request to the merge queue Dec 20, 2022
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2022
@wiedld wiedld added this pull request to the merge queue Dec 20, 2022
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 20, 2022
@wiedld wiedld added this pull request to the merge queue Dec 20, 2022
Merged via the queue into master with commit 4635a53 Dec 20, 2022
@wiedld wiedld deleted the chore/use-initLog-in-utils branch December 20, 2022 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants