Merge all commits of `async-changes` into `main` by owent · Pull Request #1413 · open-telemetry/opentelemetry-cpp

owent · 2022-05-20T06:31:44Z

This PR is created by these process below.

git checkout -b merge_main_into_async-changes open-telemetry/async-changes
git reset --hard open-telemetry/async-changes
git merge open-telemetry/main
# resolve conflicts and then
git commit -m "Merge main into async-changes"
git checkout -b merge_async-changes_into_main open-telemetry/main
git reset --hard open-telemetry/main
git merge --squash merge_main_into_async-changes
git commit -m "Merge all commits of async-changes"

Please review these changes, and I will rerun above progress and generate a new commit after some more commints are merged into main later.
@open-telemetry/cpp-approvers

Changes

[SDK] Async Batch Span/Log processor with max async support (#1306)
[EXPORTER] OTLP http exporter allow concurrency session (#1209)
[EXT] curl::HttpClient use curl_multi_handle instead of creating a thread
for every request and it's able to reuse connections now. (#1317)

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Unit tests have been added
Changes in public API reviewed

codecov · 2022-05-20T06:41:50Z

Codecov Report

Merging #1413 (6f5dee4) into main (9efb7a1) will decrease coverage by 0.65%.
The diff coverage is 79.56%.

@@            Coverage Diff             @@
##             main    #1413      +/-   ##
==========================================
- Coverage   85.31%   84.67%   -0.64%     
==========================================
  Files         154      155       +1     
  Lines        4377     4787     +410     
==========================================
+ Hits         3734     4053     +319     
- Misses        643      734      +91

Impacted Files	Coverage Δ
api/include/opentelemetry/common/spin_lock_mutex.h	`25.00% <0.00%> (-4.41%)`	⬇️
...nclude/opentelemetry/ext/http/client/http_client.h	`93.34% <ø> (ø)`
...ntelemetry/sdk/metrics/state/sync_metric_storage.h	`64.71% <ø> (ø)`
...ntelemetry/sdk/metrics/view/attributes_processor.h	`100.00% <ø> (ø)`
api/include/opentelemetry/common/timestamp.h	`81.25% <25.00%> (-18.75%)`	⬇️
ext/src/http/client/curl/http_operation_curl.cc	`76.03% <76.03%> (ø)`
ext/src/http/client/curl/http_client_curl.cc	`79.93% <79.62%> (+29.93%)`	⬆️
sdk/src/trace/batch_span_processor.cc	`91.41% <92.40%> (-1.61%)`	⬇️
...ntelemetry/ext/http/client/curl/http_client_curl.h	`95.35% <100.00%> (+3.05%)`	⬆️
...lemetry/ext/http/client/curl/http_operation_curl.h	`100.00% <100.00%> (+22.37%)`	⬆️
... and 3 more

lalitb · 2022-05-21T06:18:52Z

Oh, I was anticipating this PR to have conflicts once #1410 is merged. Sorry about this @owent, it would add to your work :(

owent · 2022-05-21T06:30:05Z

Oh, I was anticipating this PR to have conflicts once #1410 is merged. Sorry about this @owent, it would add to your work :(

That's all right.It may take a while to review this PR, there will definitely be more commits merged into main during this time.
And I will continue to work on #1291 because it will bring more conflicts.

lalitb · 2022-05-31T19:37:01Z

I was on sick leave last week, will be reviewing this over this week. sorry about that.

lalitb · 2022-06-06T20:23:40Z

Let's target this PR for the next release v1.5.0 ( tentative 17th June). Need to review this PR before that.

owent · 2022-06-07T03:08:21Z

Let's target this PR for the next release v1.5.0 ( tentative 17th June). Need to review this PR before that.

Thanks, and I have merged main branch to keep it up to date.

owent · 2022-06-13T07:10:31Z

Let's target this PR for the next release v1.5.0 ( tentative 17th June). Need to review this PR before that.

@lalitb Could you please take some time to review this PR? It's close to deadline(17th June) now.

lalitb · 2022-06-13T20:31:49Z

Let's target this PR for the next release v1.5.0 ( tentative 17th June). Need to review this PR before that.

@lalitb Could you please take some time to review this PR? It's close to deadline(17th June) now.

Yes @owent. Sorry about the delay. Metrics work internally and externally is taking lots of my bandwidth, but will definitely be completing this in the next couple of days.

lalitb · 2022-06-15T00:53:48Z

Interesting to see the outcome of this specs change - https://github.com/open-telemetry/opentelemetry-specification/pull/2452/files, what it means that exporter can upload multiple spans concurrently.

owent · 2022-06-15T02:51:51Z

Interesting to see the outcome of this specs change - https://github.com/open-telemetry/opentelemetry-specification/pull/2452/files, what it means that exporter can upload multiple spans concurrently.

According to this new specs change. should we remove the async Export() with result_callback and use only one Export() ? We can use configure to control whether to use async exporting, just like the first version of async exporting.

lalitb · 2022-06-15T05:29:42Z

Interesting to see the outcome of this specs change - https://github.com/open-telemetry/opentelemetry-specification/pull/2452/files, what it means that exporter can upload multiple spans concurrently.

According to this new specs change. should we remove the async Export() with result_callback and use only one Export() ? We can use configure to control whether to use async exporting, just like the first version of async exporting.

Actually, if these new specs changes are merged (it's already approved by multiple people), the initial PR raised by you #1209 along with libcurl-multi support would be enough as per me. Would it be the same as removing "Export() with result_callback" from this PR?

owent · 2022-06-15T13:08:32Z

Interesting to see the outcome of this specs change - https://github.com/open-telemetry/opentelemetry-specification/pull/2452/files, what it means that exporter can upload multiple spans concurrently.

According to this new specs change. should we remove the async Export() with result_callback and use only one Export() ? We can use configure to control whether to use async exporting, just like the first version of async exporting.

Actually, if these new specs changes are merged (it's already approved by multiple people), the initial PR raised by you #1209 along with libcurl-multi support would be enough as per me. Would it be the same as removing "Export() with result_callback" from this PR?

Do you think if we could do the new specs changes in another PR later?In which we can remove AsyncBatchLogProcessor and AsyncBatchSpanProcessor then.

lalitb · 2022-06-15T14:39:36Z

Do you think if we could do the new specs changes in another PR later?In which we can remove AsyncBatchLogProcessor and AsyncBatchSpanProcessor then.

Yes agree.

lalitb · 2022-06-16T02:32:57Z

Do you think if we could do the new specs changes in another PR later?In which we can remove AsyncBatchLogProcessor and AsyncBatchSpanProcessor then.

Yes agree.

@owent - This was discussed in detail in today's community meeting. We felt that with otel-specs #2452 nearing merge (should happen most probably by next week), it would be good to clean up AsyncBatch* classes and the async-export-callback method before merging this PR. This would also ensure we don't do clean-up as a breaking change - e.g If some users start having a dependency on AsyncBatch* and async-export-callback methods, the cleanup would be a problem.

I do see this as a major revert, and feel bad about it :( I can help with the cleanup on this PR to reduce your effort, please let me know?

owent · 2022-06-16T06:32:19Z

17th June

OK. I can start to remove AsyncBatch* now ,but I'm afraid it can not be finished before 17th June.

lalitb · 2022-06-16T06:53:45Z

17th June

OK. I can start to remove AsyncBatch* now ,but I'm afraid it can not be finished before 17th June.

Thanks, appreciated. No, we are not targeting this for 17th June, but for subsequent release. 17th June release would be a patch/minor release with a few bug fixes.

owent · 2022-06-16T13:56:51Z

17th June

OK. I can start to remove AsyncBatch* now ,but I'm afraid it can not be finished before 17th June.

Thanks, appreciated. No, we are not targeting this for 17th June, but for subsequent release. 17th June release would be a patch/minor release with a few bug fixes.

I raise another PR #1456 to implement open-telemetry/opentelemetry-specification#2452 .
This PR also merge the latest main branch and the real changes are all commits after b4d47e1ffd077c282a6efb86e701d383dcff7b1a.

I will update this PR after #1456 is merged.

owent · 2022-06-17T09:13:21Z

@lalitb AsyncBatch* are removed now.

ThomsonTan · 2022-06-28T18:33:44Z

+  std::lock_guard<std::mutex> lock_guard{sessions_m_};
+  sessions_.insert({session_id, session});
+
+  // FIXME: Session may leak if it do not SendRequest


Will this be fixed in future PR?

Yes, and all usage in opentelemetry-cpp will call SendRequest, this is used to notice for users if they use curl::HttpClient directly.(Though it's not designed for external usage.)

lalitb

Looks like it will take a few more days for me review it completely. The flow is a bit complex, a sequence/flow diagram would have been helpful to understand the flow and review it better. Thanks for your patience on this PR :)

lalitb

LGTM. Thanks for your patience, I think any further issues with / without feature flag - we can handle separately if they come.

lalitb · 2022-07-06T23:11:56Z

  // Send the request
-  std::unique_ptr<ResponseHandler> handler(new ResponseHandler(options_.console_debug));
-  session->SendRequest(*handler);
+  return HttpSessionData{


Nit - fix above comment, as we don't send request, but save the session in SessionData?

…_into_main

lalitb · 2022-07-08T19:21:02Z

@owent - Can you please confirm if it is good to merge?

owent · 2022-07-09T04:46:03Z

@owent - Can you please confirm if it is good to merge?

Yes, it's ready now. This PR include all commits from async-changes and already merged the latest main branch.

owent · 2022-07-09T07:19:31Z

async-changes can be removed now.

owent requested a review from a team May 20, 2022 06:31

owent changed the title ~~Merge all commits of async-changes~~ Merge all commits of async-changes into main May 20, 2022

owent force-pushed the merge_async-changes_into_main branch from c160926 to cd25708 Compare May 25, 2022 04:23

owent mentioned this pull request Jun 2, 2022

Review logger Implementation for the stable log data model. #1291

Closed

owent force-pushed the merge_async-changes_into_main branch from cd25708 to 4603f20 Compare June 7, 2022 03:07

owent mentioned this pull request Jun 10, 2022

Missing value for flag: --experimental_allow_proto3_optional #1442

Closed

owent mentioned this pull request Jun 15, 2022

Merge main into async changes #1451

Merged

owent force-pushed the merge_async-changes_into_main branch from 4603f20 to 88ee601 Compare June 15, 2022 14:58

owent force-pushed the merge_async-changes_into_main branch from 88ee601 to 6caa218 Compare June 17, 2022 06:58

lalitb reviewed Jun 22, 2022

View reviewed changes

owent force-pushed the merge_async-changes_into_main branch 2 times, most recently from 248b715 to efc89d3 Compare June 22, 2022 04:02

ThomsonTan reviewed Jun 27, 2022

View reviewed changes

Comment thread .github/workflows/stale.yml

lalitb reviewed Jun 28, 2022

View reviewed changes

Comment thread ext/src/http/client/curl/http_client_curl.cc

ThomsonTan reviewed Jun 28, 2022

View reviewed changes

Comment thread exporters/elasticsearch/src/es_log_exporter.cc Outdated

ThomsonTan reviewed Jun 28, 2022

View reviewed changes

Comment thread exporters/elasticsearch/src/es_log_exporter.cc

ThomsonTan reviewed Jun 28, 2022

View reviewed changes

Comment thread sdk/test/logs/batch_log_processor_test.cc Outdated

owent force-pushed the merge_async-changes_into_main branch from efc89d3 to 592092a Compare June 29, 2022 02:48

lalitb reviewed Jun 29, 2022

View reviewed changes

Comment thread ext/src/http/client/curl/http_client_curl.cc

Comment thread ext/src/http/client/curl/http_client_curl.cc

Comment thread ext/src/http/client/curl/http_operation_curl.cc Outdated

owent force-pushed the merge_async-changes_into_main branch from 592092a to 6f23a4c Compare July 1, 2022 07:00

This was referenced Jul 2, 2022

Merge main into async changes #1472

Merged

Increase the default size of FD_SETSIZE #1477

Closed

lalitb approved these changes Jul 7, 2022

View reviewed changes

lalitb reviewed Jul 7, 2022

View reviewed changes

Comment thread exporters/otlp/src/otlp_http_exporter.cc

owent force-pushed the merge_async-changes_into_main branch from 6f23a4c to 6f5dee4 Compare July 7, 2022 12:18

Merge branch 'merge_main_into_async-changes' into merge_async-changes…

6f5dee4

…_into_main

ThomsonTan approved these changes Jul 8, 2022

View reviewed changes

lalitb merged commit 4762910 into open-telemetry:main Jul 9, 2022

owent mentioned this pull request Jul 9, 2022

crash in ext::http::client::curl::HttpOperation::Send #1220

Closed

owent deleted the merge_async-changes_into_main branch July 9, 2022 07:24

This was referenced Jul 9, 2022

The default value of FD_SETSIZE is too small for real scenario #1476

Closed

Async/callback based interface between Span/Log Processor and Span/Log Exporter #1239

Closed

Conversation

owent commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

codecov Bot commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lalitb commented May 21, 2022

Uh oh!

owent commented May 21, 2022

Uh oh!

lalitb commented May 31, 2022

Uh oh!

lalitb commented Jun 6, 2022

Uh oh!

owent commented Jun 7, 2022

Uh oh!

owent commented Jun 13, 2022

Uh oh!

lalitb commented Jun 13, 2022

Uh oh!

lalitb commented Jun 15, 2022

Uh oh!

owent commented Jun 15, 2022

Uh oh!

lalitb commented Jun 15, 2022

Uh oh!

owent commented Jun 15, 2022

Uh oh!

lalitb commented Jun 15, 2022

Uh oh!

lalitb commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

owent commented Jun 16, 2022

Uh oh!

lalitb commented Jun 16, 2022

Uh oh!

owent commented Jun 16, 2022

Uh oh!

owent commented Jun 17, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ThomsonTan Jun 28, 2022

Choose a reason for hiding this comment

Uh oh!

owent Jun 29, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lalitb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lalitb Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

owent commented May 20, 2022 •

edited

Loading

codecov Bot commented May 20, 2022 •

edited

Loading

lalitb commented Jun 16, 2022 •

edited

Loading