Cocurrency otlp http session by owent · Pull Request #1317 · open-telemetry/opentelemetry-cpp

owent · 2022-04-05T08:23:19Z

Fixes #1243

Changes

Using curl_multi_handle to improve performance of curl::HttpClient
Keep the same behavior as before when using curl::HttpOperation::Send (sync mode), only malloc async data and bind curl_easy_handle to curl_multi_handle for curl::HttpOperation::SendAsync.
Add background thread to call curl_multi_perform and curl_multi_poll in curl::HttpClient, and if no more pending request, the thread will exit and we will create background thread again if there are more requests in the future.
Do not use system close(fd) to abort request, use curl_multi_wakeup() to wakup and do the real removing the the background thread.Also we allow to break connecting, sending or receiving data from callback of curl.

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

* add owent as reviewer * fix order

…mporality) (open-telemetry#1288)

…elemetry#1301) Signed-off-by: owent <admin@owent.net>

…emetry#1309) Signed-off-by: owent <admin@owent.net>

…ed. (open-telemetry#1299)

owent · 2022-04-05T11:35:16Z

While #1309 is already merged, why there still is only one check here?

lalitb · 2022-04-05T23:15:47Z

While #1309 is already merged, why there still is only one check here?

Probably the event is triggered form the workflow file present in the branch where merge is happening, which in this case is async-changes branch :)

owent · 2022-04-06T02:25:35Z

While #1309 is already merged, why there still is only one check here?

Probably the event is triggered form the workflow file present in the branch where merge is happening, which in this case is async-changes branch :)

Thanks, I merged main branch, and it works now.

codecov · 2022-04-06T02:29:33Z

Codecov Report

❗ No coverage uploaded for pull request base (async-changes@c614258). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             async-changes    #1317   +/-   ##
================================================
  Coverage                 ?   91.08%           
================================================
  Files                    ?      221           
  Lines                    ?     8618           
  Branches                 ?        0           
================================================
  Hits                     ?     7849           
  Misses                   ?      769           
  Partials                 ?        0

lalitb · 2022-04-06T05:03:59Z

Thanks, I merged main branch, and it works now.

Looks like this has messed up the PR, I see 56 files modified :).

owent · 2022-04-06T05:20:38Z

Thanks, I merged main branch, and it works now.

Looks like this has messed up the PR, I see 56 files modified :).

I have merged main branch, it has a lot change between async-changes and main. Maybe I should create a PR to merge main to async-changes first and then create this PR?

lalitb · 2022-04-06T06:01:16Z

Thanks, I merged main branch, and it works now.

Looks like this has messed up the PR, I see 56 files modified :).

I have merged main branch, it has a lot change between async-changes and main. Maybe I should create a PR to merge main to async-changes first and then create this PR?

Yes I think that would be better. Sorry about all this trouble.

owent · 2022-04-06T06:23:17Z

I have merged main branch, it has a lot change between async-changes and main. Maybe I should create a PR to merge main to async-changes first and then create this PR?

This PR is base on #1321 now, I think the change list will be simple after #1321 is merged.

DebajitDas · 2022-04-10T15:01:45Z

Are we trying to assign here instead of using "==" here?

Yes and thanks.it's fixed now.

DebajitDas · 2022-04-10T15:03:09Z

I am not clear why are we assigning a new thread id here?

It's not necessary, but I think it's more clear to reset callback_thread after callback is finished.

owent · 2022-04-28T14:30:04Z

I will add max_sessions_per_connection option for all OTLP HTTP exporters after #1306 is merged.

…o_async_changes Signed-off-by: owent <admin@owent.net> # Conflicts: # sdk/include/opentelemetry/sdk/metrics/aggregation/sum_aggregation.h # sdk/include/opentelemetry/sdk/metrics/data/point_data.h # sdk/include/opentelemetry/sdk/metrics/metric_exporter.h # sdk/include/opentelemetry/sdk/metrics/metric_reader.h # sdk/include/opentelemetry/sdk/metrics/state/sync_metric_storage.h # sdk/test/metrics/sync_metric_storage_test.cc

…o_async_changes

Signed-off-by: owent <admin@owent.net>

owent · 2022-05-05T06:20:15Z

@lalitb This PR is ready to be reviewed now. The Performance Alert is expired and please just ignore them.

lalitb · 2022-05-06T18:34:53Z

@lalitb This PR is ready to be reviewed now. The Performance Alert is expired and please just ignore them.

Thanks @owent. Will review this over the weekend.

DebajitDas

LGTM

lalitb · 2022-05-11T06:19:56Z

@owent one test is failing. Can we fix that before merging to feature branch.

owent · 2022-05-11T10:16:38Z

@owent one test is failing. Can we fix that before merging to feature branch.

Done. The failed test is network error. I have click rerun and it pass now.

lalitb · 2022-05-11T16:51:50Z

Merging it now. Are we good to raise PR from feature branch to main?

esigo and others added 15 commits March 21, 2022 07:02

install sdk config (open-telemetry#1273)

e072daa

Bump actions/cache from 2 to 3 (open-telemetry#1277)

6ec1b59

Add owent as an Approver (open-telemetry#1276)

b5155a5

* add owent as reviewer * fix order

Disable benchmark action failure (open-telemetry#1284)

0c9aece

metrics exemplar round 1 (open-telemetry#1264)

3c7b44b

[Metrics SDK] - fix spelling (AggregationTemporarily to AggregationTe…

2c9ce39

…mporality) (open-telemetry#1288)

fix compilation error with protobuf 3.5 (open-telemetry#1289)

91b0572

Fix span SetAttribute crash (open-telemetry#1283)

c1b9590

Synchronous Metric collection (Delta , Cumulative) (open-telemetry#1265)

a7e814a

Rename http_client_curl to opentelemetry_http_client_curl (open-t…

76c664a

…elemetry#1301) Signed-off-by: owent <admin@owent.net>

Don't show coverage annotation for pull requests (open-telemetry#1304)

2034c9b

Implement periodic exporting metric reader (open-telemetry#1286)

33d9c62

Add async-changes branch to pull_request of github action (open-tel…

48a4060

…emetry#1309) Signed-off-by: owent <admin@owent.net>

Add InstrumentationInfo and Resource to the metrics data to be export…

be75bbc

…ed. (open-telemetry#1299)

Excempt should be applied on issue instead of PR (open-telemetry#1316)

237a0b2

owent requested a review from a team April 5, 2022 08:23

owent force-pushed the cocurrency_otlp_http_session branch from 403a87e to 9af4aa1 Compare April 6, 2022 05:12

Bump codecov/codecov-action from 2.1.0 to 3 (open-telemetry#1318)

fd338cc

owent force-pushed the cocurrency_otlp_http_session branch from 9af4aa1 to 05714bb Compare April 6, 2022 06:22

owent force-pushed the cocurrency_otlp_http_session branch 2 times, most recently from d41c122 to 8628b63 Compare April 6, 2022 07:54

DebajitDas reviewed Apr 10, 2022

View reviewed changes

ThomsonTan assigned lalitb Apr 25, 2022

owent force-pushed the cocurrency_otlp_http_session branch 2 times, most recently from b65b6e4 to dc16ad1 Compare April 28, 2022 13:58

owent mentioned this pull request Apr 28, 2022

Using libcurl-multi to implement opentelemetry::ext::http::client::curl::HttpClient #1243

Closed

owent added 4 commits May 5, 2022 11:26

Merge remote-tracking branch 'opentelemetry/main' into merge_main_int…

b0e6484

…o_async_changes

Switch to multi_handle for curl::HttpClient

079dd3e

Signed-off-by: owent <admin@owent.net>

Add max_requests_per_connection for all OTLP http exporters

8e603fd

Signed-off-by: owent <admin@owent.net>

owent force-pushed the cocurrency_otlp_http_session branch from dc16ad1 to 8e603fd Compare May 5, 2022 04:40

Fix style

731565a

Signed-off-by: owent <admin@owent.net>

owent requested a review from DebajitDas May 5, 2022 06:18

DebajitDas approved these changes May 11, 2022

View reviewed changes

lalitb merged commit 08a12b5 into open-telemetry:async-changes May 11, 2022

owent deleted the cocurrency_otlp_http_session branch May 16, 2022 16:28

owent mentioned this pull request May 20, 2022

Merge all commits of async-changes into main #1413

Merged

3 tasks

Uh oh!

Conversation

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

Uh oh!

Changes

Uh oh!

owent commented Apr 5, 2022

Uh oh!

lalitb commented Apr 5, 2022

Uh oh!

owent commented Apr 6, 2022

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

lalitb commented Apr 6, 2022

Uh oh!

owent commented Apr 6, 2022

Uh oh!

lalitb commented Apr 6, 2022

Uh oh!

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

Uh oh!

Uh oh!

DebajitDas Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

owent Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

DebajitDas Apr 10, 2022

Choose a reason for hiding this comment

Uh oh!

owent Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

owent commented Apr 28, 2022

Uh oh!

owent commented May 5, 2022

Uh oh!

lalitb commented May 6, 2022

Uh oh!

DebajitDas left a comment

Choose a reason for hiding this comment

Uh oh!

lalitb commented May 11, 2022

Uh oh!

owent commented May 11, 2022

Uh oh!

lalitb commented May 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

owent commented Apr 5, 2022 •

edited

Loading

codecov Bot commented Apr 6, 2022 •

edited

Loading

owent commented Apr 6, 2022 •

edited

Loading