Add initial BEP (Build Event Protocol) support

[caass]

Description

Add support for processing BEP events. Supersedes #926.

Fixes #925

Type of change

• New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Added integration tests.

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[adam-singer]

Reviewed 8 of 8 files at r1, 5 of 5 files at r2.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: pre-commit-checks, and 1 discussions need to be resolved

---

-- commits line 1 at r2:
@caass commits will need to be squashed as a single commit

[CLAassistant]

All committers have signed the CLA.

[caass]

Reviewed 8 of 8 files at r1, 1 of 5 files at r2, 1 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 3 discussions need to be resolved

---

-- commits line 1 at r2:

Previously, adam-singer (Adam Singer) wrote…

@caass commits will need to be squashed as a single commit

Done.

---

nativelink-service/BUILD.bazel line 11 at r4 (raw file):

rust_library(
    name = "nativelink-service",
    srcs = [

FYI I'm curious why we don't glob here?

---

nativelink-service/src/bep_server.rs line 41 at r4 (raw file):

// TODO(caass): how are people expected to retrieve streams? if they need to access them via [`DigestInfo`]
// then we'll need to expose this function...

I'm curious like, how downstream consumers are meant to extract messages from the Store. At the moment, you need a &StreamId which you can then use with this function (get_stream_digest) which will compute a digest for you that you can use to retrieve the stream.

Would it make more sense to have some kind of Digestable trait akin to the Hash trait, where the downstream user can get the digest for a StreamId without needing to import this function?
Or like, impl some function on StreamId like StreamId::get_digest(&self) -> Result<DigestInfo, Error>?

Code quote:

// TODO(caass): how are people expected to retrieve streams? if they need to access them via [`DigestInfo`]
// then we'll need to expose this function...

---

nativelink-service/src/bep_server.rs line 186 at r4 (raw file):

        self.inner_publish_lifecycle_event(grpc_request.into_inner())
            .await
            .map_err(|e| e.into())

Doesn't clippy usually recommend instead using .map_err(Error::into) rather than the closure form? Not sure why it's not complaining about this....

---

nativelink-service/tests/bep_server_test.rs line 39 at r4 (raw file):

use nativelink_util::buf_channel::make_buf_channel_pair;
use nativelink_util::common::encode_stream_proto;
use nativelink_util::store_trait::{Store, StoreKey, StoreLike};

FYI I wasn't sure what the protocol was here for imports, so I went with what I saw elsewhere in the codebase: use groups (the ::{Something, SomethingElse} for leaf imports, but no group nesting (i.e. ::{Something, child_module::SomethingElse}.

Code quote:

use nativelink_proto::google::devtools::build::v1::build_event::console_output::Output;
use nativelink_proto::google::devtools::build::v1::build_event::{
    BuildEnqueued, BuildFinished, ConsoleOutput, Event, InvocationAttemptFinished,
    InvocationAttemptStarted,
};
use nativelink_proto::google::devtools::build::v1::publish_build_event_server::PublishBuildEvent;
use nativelink_proto::google::devtools::build::v1::publish_lifecycle_event_request::ServiceLevel;
use nativelink_proto::google::devtools::build::v1::stream_id::BuildComponent;
use nativelink_proto::google::devtools::build::v1::{
    build_status, BuildEvent, BuildStatus, ConsoleOutputStream, OrderedBuildEvent,
    PublishBuildToolEventStreamRequest, PublishLifecycleEventRequest, StreamId,
};
use nativelink_service::bep_server::{get_stream_digest, BepServer};
use nativelink_store::default_store_factory::store_factory;
use nativelink_store::store_manager::StoreManager;
use nativelink_util::buf_channel::make_buf_channel_pair;
use nativelink_util::common::encode_stream_proto;
use nativelink_util::store_trait::{Store, StoreKey, StoreLike};

[caass]

+@allada

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, macos-13, pre-commit-checks, windows-2022 / stable, and 3 discussions need to be resolved (waiting on @allada)

[allada]

Reviewed 8 of 8 files at r1, 2 of 5 files at r2, 1 of 3 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and 14 discussions need to be resolved

---

nativelink-service/BUILD.bazel line 11 at r4 (raw file):

Previously, caass (Cass Fridkin) wrote…

FYI I'm curious why we don't glob here?

Generally being explicit is better long term. It allows easier code searching and it is more generic for users. glob doesn't scale well as code bases get more and more complex, especially if cross-linking different modules come in to play. For example if a single file is windows only it makes glob complex and can cause problems long term.

Generally everywhere I've ever worked prefers not to use glob except for tests.

---

nativelink-service/src/bep_server.rs line 41 at r4 (raw file):

Previously, caass (Cass Fridkin) wrote…

I'm curious like, how downstream consumers are meant to extract messages from the Store. At the moment, you need a &StreamId which you can then use with this function (get_stream_digest) which will compute a digest for you that you can use to retrieve the stream.

Would it make more sense to have some kind of Digestable trait akin to the Hash trait, where the downstream user can get the digest for a StreamId without needing to import this function?
Or like, impl some function on StreamId like StreamId::get_digest(&self) -> Result<DigestInfo, Error>?

I don't think we actually need this function any more, since we now have StoreKey and StoreKey has an .into_digest() and .as_str() function that does what we need here.

Lets make a StoreKey::Str(Cow::new(format!("{}-{}-{}", stream_id.build_id, stream_id.invocation_id, stream_id.component))) and inline it.

---

nativelink-service/src/bep_server.rs line 87 at r4 (raw file):

        self.store
            .as_store_driver_pin()

nit: Try without using .as_store_driver_pin and see if it will compile.

---

nativelink-service/src/bep_server.rs line 137 at r4 (raw file):

        }
        Ok(Response::new(Box::pin(unfold(
            Some(State {

nit: Lets pull this out into it's own variable for readability.

---

nativelink-service/src/bep_server.rs line 186 at r4 (raw file):

Previously, caass (Cass Fridkin) wrote…

Doesn't clippy usually recommend instead using .map_err(Error::into) rather than the closure form? Not sure why it's not complaining about this....

Good point. I'm not sure. Try swapping it.

---

nativelink-service/tests/bep_server_test.rs line 1 at r4 (raw file):

// Copyright 2023 The NativeLink Authors. All rights reserved.

nit: Year.

---

nativelink-service/tests/bep_server_test.rs line 39 at r4 (raw file):

Previously, caass (Cass Fridkin) wrote…

FYI I wasn't sure what the protocol was here for imports, so I went with what I saw elsewhere in the codebase: use groups (the ::{Something, SomethingElse} for leaf imports, but no group nesting (i.e. ::{Something, child_module::SomethingElse}.

Yeah the way you have it is the most common way we do it. We don't have strict rules on it, but I like to have them on separate lines for leafs.

---

nativelink-service/tests/bep_server_test.rs line 84 at r4 (raw file):

/// Asserts that a gRPC request for a [`PublishLifecycleEventRequest`] is correctly dumped into a [`Store`]
#[nativelink_test]

hmmm, all the other tests have ```rs
#[cfg(test)]
mod {
...
}

Although this may not be needed. For now lets wrap all these in a module like the rest of the tests, just to ensure we are not accidentally leaking these functions into the final binaries while we investigate.

---

nativelink-service/tests/bep_server_test.rs line 95 at r4 (raw file):

        component: BuildComponent::Controller as i32,
    };
    let digest = get_stream_digest(&stream_id)?;

nit: Similar to mentioned above, lets not make this public and use direct string checking.

---

nativelink-service/tests/bep_server_test.rs line 126 at r4 (raw file):

    bep_store
        .as_store_driver_pin()

nit: Try not to use this when possible.

---

nativelink-service/tests/bep_server_test.rs line 151 at r4 (raw file):

    let (mut request_tx, mut response_stream) = async {
        // setup the request and response streams

nit: Caps and period.

---

nativelink-service/tests/bep_server_test.rs line 166 at r4 (raw file):

    let (requests, store_key) = {
        // construct some requests to send off

nit: Caps & period.

---

nativelink-service/tests/bep_server_test.rs line 275 at r4 (raw file):

    {
        // send off the requests and validate the responses

nit: ditto.

---

nativelink-service/tests/bep_server_test.rs line 276 at r4 (raw file):

    {
        // send off the requests and validate the responses
        for (i, req) in requests.iter().enumerate() {

nit: Try .enumerate().map(|(i, req)| (i + 1, req))

And add a comment saying that it starts at 1.

---

nativelink-service/tests/bep_server_test.rs line 286 at r4 (raw file):

                .err_tip(|| "While awaiting next PublishBuildToolEventStreamResponse")?;

            // First, check if the response matches what we expect

nit: period.

[caass]

Reviewed 1 of 5 files at r2.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Installation / ubuntu-22.04, Remote / large-ubuntu-22.04, Vercel, pre-commit-checks, ubuntu-20.04 / stable, vale, and 2 discussions need to be resolved

---

nativelink-service/tests/bep_server_test.rs line 39 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Yeah the way you have it is the most common way we do it. We don't have strict rules on it, but I like to have them on separate lines for leafs.

There are actually rustfmt rules for things like this in nightly, which I think we run anyway? I created an issue here

---

nativelink-service/tests/bep_server_test.rs line 84 at r4 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

hmmm, all the other tests have ```rs
#[cfg(test)]
mod {
...
}

Although this may not be needed. For now lets wrap all these in a module like the rest of the tests, just to ensure we are not accidentally leaking these functions into the final binaries while we investigate.

Done.

[allada]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 3 discussions need to be resolved

---

nativelink-service/tests/bep_server_test.rs line 63 at r5 (raw file):

#[cfg(test)]
mod publish_lifecycle_event {
    use std::borrow::Cow;

nit: Please put all but assert_eq at the root level.

Similar to what other tests do.

---

nativelink-service/tests/bep_server_test.rs line 149 at r5 (raw file):

#[cfg(test)]
mod publish_build_tool_event_stream {

nit: Also just combine these. We don't split them any more. Old tests do have them split.

[caass]

Reviewed 1 of 2 files at r5, 1 of 1 files at r7, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 1 discussions need to be resolved

---

nativelink-service/tests/bep_server_test.rs line 63 at r5 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Please put all but assert_eq at the root level.

Similar to what other tests do.

Done

---

nativelink-service/tests/bep_server_test.rs line 149 at r5 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Also just combine these. We don't split them any more. Old tests do have them split.

Done

[allada]

Dismissed @adam-singer from a discussion.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, CodeQL, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable

[allada]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, CodeQL, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable