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

Prevents reporting font update from triggering restart #74029

Closed

Conversation

tylersmalley
Copy link
Contributor

This file appears to be touched regularly on Linux (WSL2) triggering a server restart.

@elastic/kibana-reporting-services, any idea why this is happening?

This file appears to be touched regularly on Linux (WSL2) triggering a
server restart.

Signed-off-by: Tyler Smalley <tyler.smalley@elastic.co>
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@tsullivan
Copy link
Member

I get the same issue and have heard other devs say the same thing.

@elastic/kibana-reporting-services, any idea why this is happening?

I have tried searching around in the repo to find a reference to this file that would explain it, but I have not come up with an answer. @tylersmalley I assume you've tried the same thing.

I don't see anything wrong with the file attributes either.

@tsullivan
Copy link
Member

I find that dev server startup works fine even after:
rm -rf x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/assets

Could it be something about the file itself which confuses nodemon (or whatever we use for dev)?

@tsullivan
Copy link
Member

I did a test using stat and found that there aren't even changes happening to the file.

  1. Server is stopped
  2. Run stat x-pack/plugins/reporting/server/export_types/printable_pdf/lib/pdf/assets/fonts/roboto/Roboto-* > stats1.txt
  3. Run the dev server, wait for the restart due to changes detected in Roboto-Regular.ttf
  4. Run the stat command again, and save to a different file
  5. Run diff on the 2 files

In my test, there are no changes. I'm not sure why the dev server detects something is happening

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan
Copy link
Member

In src/cli/cluster/cluster_manager.ts, the onWatcherChange callback ignores the first e parameter, but it looks like that is a string that tells us what kind of modification event there was.

When I log e out in onWatcherChange, it shows add for this Roboto-Regular font file. Usually when editing and saving some file in the server code, the value of e seems to be change.

The ClusterManager class has an FSWatcher as this.watcher and it looks like it's firing it's ready event before this file is read. However, the onReady handler looks like it's trying to ignore any add events further down from getting handled.

What if we just add an early return in onWatcherChange for e === 'add'? I think that follows the general understanding of how the dev server works.

@tsullivan
Copy link
Member

What if we just add an early return in onWatcherChange for e === 'add'? I think that follows the general understanding of how the dev server works.

If we go with that solution, unfortunately that would have a breaking change to no longer cause a server restart if a file is added in the repo. A Draft PR is here: #74141

@elastic/kibana-operations any idea why the optimizer thinks this font file was added to the repo shortly after it begins watching for changes?

@tsullivan
Copy link
Member

@tylersmalley this PR is no longer applicable. I solved the issue of the font file triggering restart by making the path to the file shorter (less depth).

@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead Team:Operations Team label for Operations Team v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants