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

impl(pubsub-open-telemetry): add a publisher executable #268

Merged
merged 14 commits into from
Dec 1, 2023

Conversation

alevenberg
Copy link
Member

No description provided.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

I don't think there is any obligation to provide Bazel files for these examples. If having them makes your life easier, then by all means create them.

pubsub-open-telemetry/parse_args.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/parse_args.h Outdated Show resolved Hide resolved
pubsub-open-telemetry/publisher.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/publisher.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/WORKSPACE.bazel Outdated Show resolved Hide resolved
@alevenberg
Copy link
Member Author

I don't think there is any obligation to provide Bazel files for these examples. If having them makes your life easier, then by all means create them.

ATM I only know how to build the samples with a local version of google-cloud-cpp and bazel. I don't think it's a lot of extra to support both. So leaving both.

@coryan
Copy link
Contributor

coryan commented Nov 30, 2023

I don't think there is any obligation to provide Bazel files for these examples. If having them makes your life easier, then by all means create them.

ATM I only know how to build the samples with a local version of google-cloud-cpp and bazel. I don't think it's a lot of extra to support both. So leaving both.

Ack. FWIW:

cd $HOME/cpp-samples/pubsub-open-telemetry/
cmake --toolchain $HOME/vcpkg/scripts/buildsystems/vcpkg.cmake -G Ninja -S . -B .build 
cmake --build .build

@alevenberg
Copy link
Member Author

I don't think there is any obligation to provide Bazel files for these examples. If having them makes your life easier, then by all means create them.

ATM I only know how to build the samples with a local version of google-cloud-cpp and bazel. I don't think it's a lot of extra to support both. So leaving both.

Ack. FWIW:

cd $HOME/cpp-samples/pubsub-open-telemetry/
cmake --toolchain $HOME/vcpkg/scripts/buildsystems/vcpkg.cmake -G Ninja -S . -B .build 
cmake --build .build

But this will only used the version in vcpkg right?

I'm looking for something like bazel run //:quickstart --override_repository=google_cloud_cpp=$HOME/your-path-to-the-repo/google-cloud-cpp where I can override the repository to any checked out version of google_cloud_cpp (not just a version, but the commit at head)

@coryan
Copy link
Contributor

coryan commented Nov 30, 2023

I don't think there is any obligation to provide Bazel files for these examples. If having them makes your life easier, then by all means create them.

ATM I only know how to build the samples with a local version of google-cloud-cpp and bazel. I don't think it's a lot of extra to support both. So leaving both.

Ack. FWIW:

cd $HOME/cpp-samples/pubsub-open-telemetry/
cmake --toolchain $HOME/vcpkg/scripts/buildsystems/vcpkg.cmake -G Ninja -S . -B .build 
cmake --build .build

But this will only used the version in vcpkg right?

Yes.

I'm looking for something like bazel run //:quickstart --override_repository=google_cloud_cpp=$HOME/your-path-to-the-repo/google-cloud-cpp where I can override the repository to any checked out version of google_cloud_cpp (not just a version, but the commit at head)

I see. That is possible but waaay too complicated and brittle in CMake.

@coryan coryan marked this pull request as ready for review November 30, 2023 22:06
@coryan coryan requested a review from a team as a code owner November 30, 2023 22:06
iot/mqtt-ciotc/setup_device.sh Outdated Show resolved Hide resolved
pubsub-open-telemetry/README.md Outdated Show resolved Hide resolved
speech/api/async_transcribe.cc Outdated Show resolved Hide resolved
speech/api/async_transcribe.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/publisher.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/publisher.cc Outdated Show resolved Hide resolved
@alevenberg
Copy link
Member Author

#268 (comment)

This example is not meant for reading consumption (for a user), that is what the quickstart is for. This one is to easily generate traces with different configurations without having to manually change code.

I see what you are saying though and I don't think we need the basic tracing example. I will remove it, and just set a default for the arg.

@alevenberg alevenberg requested review from dbolduc and coryan December 1, 2023 18:08
pubsub-open-telemetry/parse_args.cc Outdated Show resolved Hide resolved
pubsub-open-telemetry/parse_args.h Outdated Show resolved Hide resolved
pubsub-open-telemetry/publisher.cc Outdated Show resolved Hide resolved
@alevenberg alevenberg enabled auto-merge (squash) December 1, 2023 18:24
Comment on lines +78 to +82
for (auto& id : ids) try {
std::cout << "Sent message with id: " << id.get() << "\n";
} catch (std::exception const& ex) {
std::cout << "Error in publish: " << ex.what() << "\n";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. What throws here, and, if it does, what is the state of std::cout?

Bottom line, it is probably best to separate the throwing and output parts.

@alevenberg alevenberg merged commit 843e474 into GoogleCloudPlatform:main Dec 1, 2023
6 checks passed
@alevenberg alevenberg deleted the samples-2 branch December 1, 2023 18:56
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.

4 participants