Skip to content

Providing an abstract HTTP interface.#370

Merged
reyang merged 21 commits intoopen-telemetry:masterfrom
lalitb:http-client-api
Oct 24, 2020
Merged

Providing an abstract HTTP interface.#370
reyang merged 21 commits intoopen-telemetry:masterfrom
lalitb:http-client-api

Conversation

@lalitb
Copy link
Copy Markdown
Member

@lalitb lalitb commented Oct 20, 2020

This PR brings the HTTP client API to be used across various exporters ( eg zipkin, azure monitor ) to communicate with remove telemetry servers/collectors.

In continuation with previous #309 PR which I have closed now due to copyright/license issues.

This closes #6

@maxgolov @pyohannes @ThomsonTan - please help reviewing this.

@lalitb lalitb requested a review from a team October 20, 2020 13:35
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 20, 2020

Codecov Report

Merging #370 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #370      +/-   ##
==========================================
- Coverage   94.91%   94.88%   -0.03%     
==========================================
  Files         155      155              
  Lines        6882     6882              
==========================================
- Hits         6532     6530       -2     
- Misses        350      352       +2     
Impacted Files Coverage Δ
sdk/include/opentelemetry/sdk/metrics/controller.h 90.32% <0.00%> (-6.46%) ⬇️

@lalitb lalitb mentioned this pull request Oct 20, 2020
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Copy link
Copy Markdown
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with some suggestions.

Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. This will be an important part of our SDK.

Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h
lalitb and others added 5 commits October 21, 2020 23:31
Co-authored-by: Johannes Tax <jtax@newrelic.com>
Co-authored-by: Johannes Tax <jtax@newrelic.com>
Co-authored-by: Johannes Tax <jtax@newrelic.com>
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Copy link
Copy Markdown
Contributor

@pyohannes pyohannes left a comment

Choose a reason for hiding this comment

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

I have some smaller questions. But pending those answered, this PR is ready to be merged and I'll approve it.

Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
Comment thread sdk/include/opentelemetry/sdk/common/http_client.h Outdated
lalitb and others added 4 commits October 23, 2020 21:05
Co-authored-by: Johannes Tax <jtax@newrelic.com>
Co-authored-by: Johannes Tax <jtax@newrelic.com>
Co-authored-by: Johannes Tax <jtax@newrelic.com>
@reyang reyang merged commit 942bff4 into open-telemetry:master Oct 24, 2020

void OnError(nostd::string_view err) noexcept override
{
std::cout << " Error:" << err;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not std:cerr?

Delete
};

enum class SessionState
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There are plenty of errors, e.g., libcurl https://curl.haxx.se/libcurl/c/libcurl-errors.html - some are specific to libcurl, others are specific to non HTTP/HTTPS protocols, but many would be found in any HTTP library

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Right. http_client implementer for libcurl, winInet, winhttp and others need to ensure to map all those library-specific errors to http_client error states.

kxyr pushed a commit to open-o11y/opentelemetry-cpp that referenced this pull request Oct 29, 2020
GerHobbelt pushed a commit to GerHobbelt/opentelemetry-cpp that referenced this pull request Aug 31, 2025
…s-create-or-update-comment-digest

Update peter-evans/create-or-update-comment digest to f2fea6b
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.

HTTP client API for exporter developers

7 participants