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

cleanup(rest): only trace if enabled #12098

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

dbolduc
Copy link
Member

@dbolduc dbolduc commented Jul 14, 2023

We were always including the REST tracing client. Now, only include it if tracing is enabled, as determined by the Options.

Make(Default|Pooled)RestClient(...) are one of those interfaces (like Make*Connection()) that are difficult to cover with unit tests. If someone asks, I can try to cover it in an integration test.


NOTE: I also changed a

std::make_shared<DefaultCurlHandleFactory>(options);

to a

GetDefaultCurlHandleFactory(options);

The difference is that GetDefaultCurlHandleFactory(options) checks if CARootsFilePathOption is empty, before using it and CAPathOption:

if (!options.get<CARootsFilePathOption>().empty()) {

I think that is the one we want. @coryan can you speak to this?


This change is Reviewable

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Patch coverage: 92.30% and project coverage change: -0.01 ⚠️

Comparison is base (08e52b4) 93.68% compared to head (cfb9cfa) 93.68%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12098      +/-   ##
==========================================
- Coverage   93.68%   93.68%   -0.01%     
==========================================
  Files        2012     2012              
  Lines      175713   175716       +3     
==========================================
- Hits       164619   164616       -3     
- Misses      11094    11100       +6     
Impacted Files Coverage Δ
google/cloud/internal/curl_rest_client.cc 98.57% <92.30%> (+2.22%) ⬆️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dbolduc dbolduc marked this pull request as ready for review July 14, 2023 18:06
@dbolduc dbolduc requested a review from a team as a code owner July 14, 2023 18:06
@dbolduc dbolduc merged commit ff1a12b into googleapis:main Jul 14, 2023
58 checks passed
@dbolduc dbolduc deleted the cleanup-rest-tracing-client branch July 14, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants