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

{bazci,dev}: send build events to beaver hub #88219

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

healthy-pod
Copy link
Contributor

@healthy-pod healthy-pod commented Sep 20, 2022

This code change lets bazci and dev send build events to beaver hub so
we can start collecting data about builds in CI and locally.

Release note: None
Epic: CRDB-8350

@healthy-pod healthy-pod requested a review from a team as a code owner September 20, 2022 07:53
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@healthy-pod healthy-pod added the do-not-merge bors won't merge a PR with this label. label Sep 20, 2022
@healthy-pod healthy-pod force-pushed the collect-ci-data branch 3 times, most recently from 577041f to 7bcbd00 Compare October 14, 2022 18:40
@healthy-pod healthy-pod changed the title bazci: send build events to beaver hub {bazci,dev}: send build events to beaver hub Oct 14, 2022
@healthy-pod
Copy link
Contributor Author

This is now ready for review but shouldn't be merged until beaver hub PR is merged and applied. Only change needed here is updating beaverHubServerEndpoint once we have it.

Copy link
Collaborator

@rickystewart rickystewart left a comment

Choose a reason for hiding this comment

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

I think the bazci change will conflict with #89845, which will stop bazci from producing a --build_event_binary_file. Can you rebase atop that change and make sure it's still working?

if err != nil {
return err
}
args = append(args, fmt.Sprintf("--build_event_binary_file=%s", filepath.Join(workspace, bepFileBasename)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, I would rather you just put this in a temporary directory rather than right in the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right but how can we pass the tempdir name back to main where we call sendBepDataToBeaverHubIfNeeded? I see three options:

  • keep it like it is
  • mutable global variable
  • call sendBepDataToBeaverHubIfNeeded at the end of dev.build and dev.test, while still not failing or returning any beaver hub errors (just logging them and returning the build/test err if any)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand why it's so important the sendBepDataToBeaverHubIfNeeded call is all the way up in main(). Seems like the calls can just be in build() or test(), given those are the only two places where we produce these files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So having a temporary dir sounds good to me and I made the change but we have an issue. How can we deal with this dynamic output from testcases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a --data-driven-test flag, ready for another look now.

@healthy-pod healthy-pod force-pushed the collect-ci-data branch 3 times, most recently from 577c2b7 to 642ffe4 Compare December 2, 2022 00:06
@healthy-pod healthy-pod removed the do-not-merge bors won't merge a PR with this label. label Dec 2, 2022
pkg/cmd/dev/build.go Outdated Show resolved Hide resolved
pkg/cmd/bazci/bazci.go Show resolved Hide resolved
@healthy-pod healthy-pod force-pushed the collect-ci-data branch 3 times, most recently from cef7a2b to 5175efb Compare December 2, 2022 19:32
Copy link
Collaborator

@rickystewart rickystewart left a comment

Choose a reason for hiding this comment

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

Instead of adding a new flag, one thing you might try is making a global variable to mark whether this is a test. IIRC we used to have this logic in dev but it was removed due to being no longer needed? Anyway, here's what you can do:

  1. In some file (e.g. util.go), add var isTesting bool as a global variable. This will default to false for all uses in dev.
  2. Update to refer to isTesting instead of dataDrivenTestIgnoreTemporaryPaths
  3. In pkg/cmd/dev/datadriven_test.go, add func init() { isTesting = true }. This will ensure that the variable is set only for test builds.

This way you get the behavior during tests and you don't have to update anything unnecessary in the datadriven test files.

Another option is depending on pkg/util/buildutil and checking buildutil.CrdbTestBuild, although you need an extra dependency. It's a tiny dependency so this is probably not a huge deal for dev.

@healthy-pod
Copy link
Contributor Author

Another option is depending on pkg/util/buildutil and checking buildutil.CrdbTestBuild, although you need an extra dependency. It's a tiny dependency so this is probably not a huge deal for dev.

Nice, I went with this one.

@@ -120,6 +121,20 @@ var allBuildTargets = func() []string {
}()

func (d *dev) build(cmd *cobra.Command, commandLine []string) error {
// tmpDir will contain the build event binary file if produced.
tmpDir, err := os.MkdirTemp("", "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Consider not creating this directory at all (and not cleaning it up) if buildutil.CrdbTestBuild is set.

@rickystewart
Copy link
Collaborator

I think you'll have to update the disallowed_imports_test in pkg/cmd/dev/BUILD.bazel to allow //pkg/util/buildutil, BTW.

@healthy-pod
Copy link
Contributor Author

healthy-pod commented Dec 2, 2022

I think you'll have to update the disallowed_imports_test in pkg/cmd/dev/BUILD.bazel to allow //pkg/util/buildutil, BTW.

Looks like it is already allowed on master.

Edit: oh no it's not nvm..

This code change lets `bazci` and `dev` send build events to beaver hub so
we can start collecting data about builds in CI and locally.

Release note: None
Epic: CRDB-8350
@healthy-pod
Copy link
Contributor Author

TFTR!

bors r=rickystewart

@craig craig bot merged commit 865ad56 into cockroachdb:master Dec 2, 2022
@craig
Copy link
Contributor

craig bot commented Dec 2, 2022

Build succeeded:

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