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(NA): @kbn/pm new commands to support development on Bazel packages #96465

Merged

Conversation

mistic
Copy link
Member

@mistic mistic commented Apr 7, 2021

One step forward on #69706

That PR should close the gap about the problem raised by @spalger at #96227 (comment)

It introduces two new commands (yarn kbn build-bazel and yarn kbn watch-bazel) that can be used to build and watch packages already migrated into Bazel. After discussing with @spalger and @tylersmalley we've decided to go that simple way for now and evaluate how well it work for us. Additionally I've also added warnings both on yarn kbn run and yarn kbn watch.

It already adds documentation on the getting started development guide about this matter.

@mistic mistic added chore Team:Operations Team label for Operations Team v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.13.0 labels Apr 7, 2021
@mistic mistic requested a review from a team as a code owner April 7, 2021 17:32
@elasticmachine
Copy link
Contributor

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

@mistic mistic mentioned this pull request Apr 7, 2021
23 tasks
@mistic mistic added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 7, 2021
@mistic mistic enabled auto-merge (squash) April 7, 2021 17:34
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

I tested process watching and building on Windows and didn't run into any issues. I know tylersmalley and spalger were more involved architecturally so leaving this as a comment.

@mistic mistic requested review from spalger and tylersmalley April 7, 2021 22:37
@mistic

This comment has been minimized.

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

I'm on board with this approach to smart bazel commands that just build what's necessary, but I find the log output to be pretty sub-par. Can we spend a little time on the developer experience here before we start transitioning everything over to this?

yarn kbn build-bazel

Success case:
image
I'd really like to know more about what was built, ideally broken down by package, and less about the inner workings about Bazel.

Error case:
image

This error output is much worse than the (admittedly not great) error output currently produced by kbn/pm:

image
I really think we should be doing better than we are currently, or at least have a plan for how we're going to do better.

yarn kbn watch-bazel

Success case:
image
I'm a little concerned that it takes 20 seconds to initialize the environment so that we can run the build just for @elastic/datemath. I'm sure some of the problem is that we're calling yarn install in order to load the packages, but is that 20 seconds going to turn into 100 seconds when we have everything in the repo running in bazel?

Also, running yarn install twice is very strange, and again, the output is almost completely irrelevant to the package author and is instead all focused on the Bazel internals, which I don't really like.

This is all in comparison to this on master, which takes about 2 seconds:
image

Error case:
image

This suffers from the same problems as the standard build, the error is totally buried by irrelevant logs

@mistic mistic disabled auto-merge April 8, 2021 23:52
@mistic
Copy link
Member Author

mistic commented Apr 9, 2021

@spalger I feel like I've address all of your feedback except the error case logging clutter. I'll take a better look on Monday about what our options are for the error logs when building the packages but I feel like for now that is probably the best we can do without spending too much time on replacing the pkg_npm rule.

@mistic
Copy link
Member Author

mistic commented Apr 9, 2021

@elasticmachine merge upstream

@alexeagle
Copy link

Hey, just dropping by to say it's great you're adopting Bazel + rules_nodejs and I'm interested to improve the developer experience to make the logging work the way you need :)

@mistic
Copy link
Member Author

mistic commented Apr 9, 2021

@alexeagle thanks for stopping by ❤️ ! I'll work with you offline in order to give you an idea around where we think logging could be improved 👍

@mistic
Copy link
Member Author

mistic commented Apr 12, 2021

@elasticmachine merge upstream

@mistic mistic requested a review from a team as a code owner April 12, 2021 17:17
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

The latest changes reduce the time watch-bazel takes to initialize and include a summary of the packages that were built in build-bazel. If we can also tell ts_project to use tsc --pretty and remove the double logging happening on errors in build-bazel then I think we would have implemented all that we can to improve the logging output from our end.

I still think there's a lot of noise in these logs coming from Bazel but I'm on board with iterating on that with the Bazel team, especially in these areas:

  1. I think that green highlights are best reserved for success messages, rather than used for every INFO: message
    image

  2. Parts of this error are truncated to avoid being too verbose, but I think the truncation is more clever than useful. I'd argue that Bazel should just strip the CLI info from this and focus on the most informative bits, then leave the other info to more verbose logging levels

     info [bazel] ERROR: /Users/spalger/kbn-dev/master/kibana/packages/elastic-datemath/BUILD.bazel:41:11: Compiling TypeScript project //packages/elastic-datemath:tsc [tsc -p packages/elastic-datemath/tsconfig.json] failed: (Exit 2): tsc.sh failed: error executing command bazel-out/host/bin/external/npm/typescript/bin/tsc.sh --pretty --project packages/elastic-datemath/tsconfig.json --outDir bazel-out/darwin-fastbuild/bin/packages/elastic-datemath/target --rootDir ... (remaining 6 argument(s) skipped)
     info [bazel]
     info [bazel] Use --sandbox_debug to see verbose messages from the sandbox tsc.sh failed: error executing command bazel-out/host/bin/external/npm/typescript/bin/tsc.sh --pretty --project packages/elastic-datemath/tsconfig.json --outDir bazel-out/darwin-fastbuild/bin/packages/elastic-datemath/target --rootDir ... (remaining 6 argument(s) skipped)
     info [bazel]
     info [bazel] Use --sandbox_debug to see verbose messages from the sandbox
    

    vs

    info [bazel] ERROR: Compiling TypeScript project //packages/elastic-datemath:tsc failed: (Exit 2)
    info [bazel] Use --sandbox_debug to see verbose messages from the sandbox
    
  3. I'm probably wrong here but my understanding is that --sandbox_debug impacts the way things are handled on the filesystem which seems like a lot more than a flag enabling "verbose messages". Additionally, it doesn't work with iBazel, and is highlighted with empty lines which make it stand out a lot, resulting in more noise.

@mistic
Copy link
Member Author

mistic commented Apr 12, 2021

@spalger @tylersmalley I think I've mainly walked across all the left feedback and I really think we are in a much better place there we were a couple days before. I will raise the rest of the logging concerns within the Bazel team as discussed and I hope we can continue to improve on this field. Thank you very much for all your feedback and inputs and thanks for helping this project getting even better @spalger

@mistic mistic enabled auto-merge (squash) April 12, 2021 22:27
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@mistic mistic merged commit e3f5249 into elastic:master Apr 13, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Apr 13, 2021
…ges (elastic#96465)

* chore(NA): add warnings both to run and watch commands about Bazel built packages

* chore(NA): add new commands to build and watch bazel packages

* docs(NA): add documentation about how to deal with bazel packages

* chore(NA): addressed majority of the feedback received except for improved error logging

* chore(NA): disable ibazel info notification.

* chore(NA): remove iBazel notification

* chore(NA): remove iBazel notification - kbn pm dist

* chore(NA): move show_results option to kbn-pm only

* chore(NA): patch build bazel command to include packages target list

* chore(NA): add pretty logging for elastic-datemath

* chore(NA): remove double error output from commands ran with Bazel

* fix(NA): include simple error message to preserve subprocess failure state

* docs(NA): missing docs about how to independentely watch non bazel packages

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 13, 2021
…ges (#96465) (#96911)

* chore(NA): add warnings both to run and watch commands about Bazel built packages

* chore(NA): add new commands to build and watch bazel packages

* docs(NA): add documentation about how to deal with bazel packages

* chore(NA): addressed majority of the feedback received except for improved error logging

* chore(NA): disable ibazel info notification.

* chore(NA): remove iBazel notification

* chore(NA): remove iBazel notification - kbn pm dist

* chore(NA): move show_results option to kbn-pm only

* chore(NA): patch build bazel command to include packages target list

* chore(NA): add pretty logging for elastic-datemath

* chore(NA): remove double error output from commands ran with Bazel

* fix(NA): include simple error message to preserve subprocess failure state

* docs(NA): missing docs about how to independentely watch non bazel packages

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants