fix(pubsub): get future value before returning #13241

alevenberg · 2023-12-06T18:34:17Z

Closes #13240 and #13244

This change is

coryan

The change looks good, but some kind of test was (and remains) missing if this made it so far.

codecov · 2023-12-06T18:56:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e7df5df) 93.04% compared to head (ebec756) 93.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13241      +/-   ##
==========================================
+ Coverage   93.04%   93.05%   +0.01%     
==========================================
  Files        2136     2137       +1     
  Lines      186524   186562      +38     
==========================================
+ Hits       173545   173612      +67     
+ Misses      12979    12950      -29

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dbolduc

Is this really a fix? @coryan would know better, but I thought we implicitly get the future in this case.

google-cloud-cpp/google/cloud/internal/future_then_impl.h

Lines 42 to 44 in 803c09a

    
           template <typename T> 
        
           inline future<T>::future(future<future<T>>&& rhs) 
        
               : future<T>(rhs.then([](future<future<T>> f) { return f.get(); })) {}

If so, this is just a cleanup.

alevenberg · 2023-12-06T21:07:47Z

I'm not sure what test would cover this.... Do you have any ideas @coryan?

coryan · 2023-12-06T21:20:53Z

I'm not sure what test would cover this.... Do you have any ideas @coryan?

No simple ideas, just some thoughts on how to start thinking about it: do you have a way to reproduce #13240 ? Can you turn that repro into a unit test?

alevenberg · 2023-12-06T21:53:04Z

#13244

coryan

If you prefer to merge this and then add the regression test please go ahead.

alevenberg · 2023-12-07T14:01:35Z

Tested locally with: bazel test //google/cloud/pubsub/integration_tests:publisher_integration_test --test_env=GOOGLE_CLOUD_PROJECT=cloud-cpp-testing-resources --test_env=HOME=$HOME

google/cloud/pubsub/integration_tests/publisher_integration_test.cc

dbolduc · 2023-12-07T19:10:34Z

google/cloud/pubsub/integration_tests/publisher_integration_test.cc

+using ::google::cloud::testing_util::DisableTracing;
+using ::google::cloud::testing_util::EnableTracing;
+
+TEST_F(PublisherIntegrationTest, TracingEnabled) {


We should install the span catcher in these tests, and set some expectations on the results. While something like #13034 would be nice, it might take some brainpower to design. I think we should start by verifying spans that we know are created all of the time. Something like:

EXPECT_THAT(span_catcher->GetSpans(), AllOf( Contains(SpanNamed("publish")), Contains(SpanNamed("google.pubsub.v1.PublisherClient/Publish"))));

We can worry about parent/child relationships, sequence of spans, conditional spans (e.g. the backoffs) later.

I don't think we want a SpanCatcher here. That's what the publisher_connection is for.

So your test verifies that if you make a client with tracing enabled and use it, the application does not crash.

Why not also verify that if you make a client with tracing enabled and use it, it produces spans?

Because that's what the publisher_connection_test does. Then I would have 2 of the same tests.

For an end to end test of traces generated, I think there's a better spot then here. Maybe something that creates a publisher, exports the span data to a .txt file, then diffs it.

I don't think we want a SpanCatcher here.

Also, if we don't set an exporter, all of the OTel functions will be no-ops. That will probably limit the coverage we are hoping to achieve from an end-to-end integration test.

Because that's what the publisher_connection_test does.

Ok, I did not realize we decorated the mock stub in those tests

google-cloud-cpp/google/cloud/pubsub/publisher_connection_test.cc

Line 376 in ad86a22

SpanNamed("google.pubsub.v1.Publisher/Publish"),

Still, the production code path is slightly different from the test code path. For integration tests, we want to be using the clients and public APIs the way an application would.

For an end to end test of traces generated, I think there's a better spot then here.

Yeah, you could have a pubsub/integration_tests/publisher_tracing_test.cc that makes all different sorts of publisher clients, uses them against production / an emulator, and verifies the spans produced. I am thinking like one TEST() per client configuration.

Maybe something that creates a publisher, exports the span data to a .txt file, then diffs it.

You are going to have to explain this one to me. Why are we exporting to a .txt file, when we have access to the spans programmatically? What are we diffing the .txt file against? Are we writing the span's creation time, which would change every time we run the program?

I don't think we want a SpanCatcher here.

Also, if we don't set an exporter, all of the OTel functions will be no-ops. That will probably limit the coverage we are hoping to achieve from an end-to-end integration test.

Yes that makes sense.

Because that's what the publisher_connection_test does.

Ok, I did not realize we decorated the mock stub in those tests

google-cloud-cpp/google/cloud/pubsub/publisher_connection_test.cc

Line 376 in ad86a22

SpanNamed("google.pubsub.v1.Publisher/Publish"),

Still, the production code path is slightly different from the test code path. For integration tests, we want to be using the clients and public APIs the way an application would.

Is it that different? I'm not sure... Also, this is test is not using the client in any special way. I really think it has the same coverage as publisher_connection_test. At least if the only assertion we make is

EXPECT_THAT(span_catcher->GetSpans(), AllOf( Contains(SpanNamed("publish")), Contains(SpanNamed("google.pubsub.v1.PublisherClient/Publish"))));

For an end to end test of traces generated, I think there's a better spot then here.

Yeah, you could have a pubsub/integration_tests/publisher_tracing_test.cc that makes all different sorts of publisher clients, uses them against production / an emulator, and verifies the spans produced. I am thinking like one TEST() per client configuration.

This sounds like a better idea. WDYM by client configuration? Different options?

Maybe something that creates a publisher, exports the span data to a .txt file, then diffs it.

You are going to have to explain this one to me. Why are we exporting to a .txt file, when we have access to the spans programmatically? What are we diffing the .txt file against? Are we writing the span's creation time, which would change every time we run the program?

It does not need to be this. What I meant was a solution to #13034

WDYM by client configuration? Different options?

👍

What I meant was a solution to #13034

I am trying to understand the solution, but failing to do so.

This is just my guess about what happens under the hood in something like https://tracetest.io/

stale

dbolduc · 2023-12-08T16:37:45Z

google/cloud/pubsub/integration_tests/publisher_integration_test.cc

+    if (using_emulator) {
+      options_ = Options{}
+                     .set<UnifiedCredentialsOption>(MakeInsecureCredentials())
+                     .set<internal::UseInsecureChannelOption>(true);


TIL: UseInsecureChannelOption

This reverts commit a061069.

alevenberg · 2023-12-08T19:37:27Z

I think any reviews are stale, so requested a re-review

google/cloud/pubsub/integration_tests/publisher_integration_test.cc

fix(pubsub): get future value before returning

39f72bc

alevenberg requested a review from a team as a code owner December 6, 2023 18:34

alevenberg temporarily deployed to internal December 6, 2023 18:34 — with GitHub Actions Inactive

product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label Dec 6, 2023

coryan reviewed Dec 6, 2023

View reviewed changes

dbolduc previously approved these changes Dec 6, 2023

View reviewed changes

coryan previously approved these changes Dec 7, 2023

View reviewed changes

test(pubsub): add regression integration test

0c33f9b

alevenberg temporarily deployed to internal December 7, 2023 14:01 — with GitHub Actions Inactive

refactor

73e76cf

alevenberg temporarily deployed to internal December 7, 2023 14:52 — with GitHub Actions Inactive

devbww reviewed Dec 7, 2023

View reviewed changes

google/cloud/pubsub/integration_tests/publisher_integration_test.cc Outdated Show resolved Hide resolved

dbolduc reviewed Dec 7, 2023

View reviewed changes

fix options

13b38d7

alevenberg temporarily deployed to internal December 7, 2023 21:19 — with GitHub Actions Inactive

alevenberg added 2 commits December 8, 2023 09:25

use old admin

a061069

Merge remote-tracking branch 'refs/remotes/origin/fix-3' into fix-3

17d470f

alevenberg temporarily deployed to internal December 8, 2023 14:26 — with GitHub Actions Inactive

checkers

4b82377

alevenberg temporarily deployed to internal December 8, 2023 16:08 — with GitHub Actions Inactive

dbolduc approved these changes Dec 8, 2023

View reviewed changes

alevenberg mentioned this pull request Dec 8, 2023

test(pubsub): add tracing enabled tests for blocking publisher #13251

Merged

Merge branch 'main' into fix-3

08bde8a

alevenberg added 3 commits December 8, 2023 14:23

Revert "use old admin"

932c9a9

This reverts commit a061069.

remove ifdefs

104630a

remove default credentials

d256044

alevenberg temporarily deployed to internal December 8, 2023 19:28 — with GitHub Actions Inactive

alevenberg requested a review from dbolduc December 8, 2023 19:37

coryan approved these changes Dec 8, 2023

View reviewed changes

google/cloud/pubsub/integration_tests/publisher_integration_test.cc Outdated Show resolved Hide resolved

whoops s/true/false

ebec756

alevenberg temporarily deployed to internal December 8, 2023 19:43 — with GitHub Actions Inactive

alevenberg enabled auto-merge (squash) December 8, 2023 19:43

alevenberg merged commit 32f90f1 into googleapis:main Dec 8, 2023
59 checks passed

alevenberg deleted the fix-3 branch December 8, 2023 20:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(pubsub): get future value before returning #13241

fix(pubsub): get future value before returning #13241

alevenberg commented Dec 6, 2023 •

edited

Loading

coryan left a comment

codecov bot commented Dec 6, 2023 •

edited

Loading

dbolduc left a comment

alevenberg commented Dec 6, 2023

coryan commented Dec 6, 2023

alevenberg commented Dec 6, 2023

coryan left a comment

alevenberg commented Dec 7, 2023

dbolduc Dec 7, 2023 •

edited

Loading

alevenberg Dec 7, 2023 •

edited

Loading

dbolduc Dec 7, 2023

alevenberg Dec 7, 2023

alevenberg Dec 7, 2023

dbolduc Dec 7, 2023

alevenberg Dec 7, 2023 •

edited

Loading

dbolduc Dec 7, 2023

alevenberg Dec 7, 2023

dbolduc Dec 8, 2023

alevenberg commented Dec 8, 2023

	template <typename T>
	inline future<T>::future(future<future<T>>&& rhs)
	: future<T>(rhs.then([](future<future<T>> f) { return f.get(); })) {}

fix(pubsub): get future value before returning #13241

fix(pubsub): get future value before returning #13241

Conversation

alevenberg commented Dec 6, 2023 • edited Loading

coryan left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

dbolduc left a comment

Choose a reason for hiding this comment

alevenberg commented Dec 6, 2023

coryan commented Dec 6, 2023

alevenberg commented Dec 6, 2023

coryan left a comment

Choose a reason for hiding this comment

alevenberg commented Dec 7, 2023

dbolduc Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

alevenberg Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

dbolduc Dec 7, 2023

Choose a reason for hiding this comment

alevenberg Dec 7, 2023

Choose a reason for hiding this comment

alevenberg Dec 7, 2023

Choose a reason for hiding this comment

dbolduc Dec 7, 2023

Choose a reason for hiding this comment

alevenberg Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

dbolduc Dec 7, 2023

Choose a reason for hiding this comment

alevenberg Dec 7, 2023

Choose a reason for hiding this comment

dbolduc Dec 8, 2023

Choose a reason for hiding this comment

alevenberg commented Dec 8, 2023

alevenberg commented Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

dbolduc Dec 7, 2023 •

edited

Loading

alevenberg Dec 7, 2023 •

edited

Loading

alevenberg Dec 7, 2023 •

edited

Loading