-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
opentelemetry tracer: add OTLP/HTTP exporter #29207
Merged
wbpcode
merged 49 commits into
envoyproxy:main
from
dynatrace-oss-contrib:otlp-http-exporter-continue
Oct 18, 2023
Merged
Changes from all commits
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
c6ece86
Refactor gRPC exporter to use general base class we can later reuse
AlexanderEllis b59764e
Update OpenTelemetryConfig with HTTP-related config
AlexanderEllis ad99eb0
Write and wire up HTTP trace exporter
AlexanderEllis 15fa601
Code formatting
AlexanderEllis 8feddf1
Proto format
AlexanderEllis 0a4f183
Add basic tests
AlexanderEllis 5e4f9a6
code format
AlexanderEllis c058475
Add additional counters for http exporter and test
AlexanderEllis 1a6d2fc
Formatting
AlexanderEllis bf51b6e
Coverage improvements
AlexanderEllis 40cd7fc
Formatting
AlexanderEllis 0a20370
Add higher level Driver test for HTTP exporting
AlexanderEllis a498727
More formatting
AlexanderEllis ccf877c
Clang tidy
AlexanderEllis 6793f2f
Refactor config and add HTTP headers
joaopgrassi ab935a7
Modify tests
joaopgrassi 03edac8
Fix lint errors
joaopgrassi c3e8b81
Revert oneof in proto config
joaopgrassi f507a0f
PR suggestions
joaopgrassi 6815fb5
Add HttpService config type
joaopgrassi ec2a0d5
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 8d1d23a
Fix format issues
joaopgrassi f883976
Fix lint/format/docs issues
joaopgrassi 0b5b8bf
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 2da074d
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 1389804
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 2b6b267
Hostname -> Authority
joaopgrassi 3be83f8
Use HttpUri type
joaopgrassi 38adbf1
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 2a5d544
PR suggestions
joaopgrassi 935ad0c
Improve log messages
joaopgrassi 9ec6806
Fix format
joaopgrassi 9b5e47a
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi c0f39d7
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi f852967
PR suggestions
joaopgrassi 7ea4d0b
PR suggestions
joaopgrassi ba5d943
Revert stats for http exporter
joaopgrassi 38bd4ac
Fix spelling
joaopgrassi 3d07358
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi ec3911b
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi 8c7ca70
Add changelog
joaopgrassi efcd78a
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi e339153
Properly manage in-flight http requests in the exporter
joaopgrassi e7932fb
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi aacb3f4
Add proto migrate oneof annotation
joaopgrassi c8f74c3
Clarify http headers do not support formatting
joaopgrassi e00f32b
Merge remote-tracking branch 'upstream/main' into otlp-http-exporter-…
joaopgrassi cb08695
Prepare headers when creating the HTTP exporter
joaopgrassi 97f6870
Revert test change
joaopgrassi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
syntax = "proto3"; | ||
|
||
package envoy.config.core.v3; | ||
|
||
import "envoy/config/core/v3/base.proto"; | ||
import "envoy/config/core/v3/http_uri.proto"; | ||
|
||
import "udpa/annotations/status.proto"; | ||
import "validate/validate.proto"; | ||
|
||
option java_package = "io.envoyproxy.envoy.config.core.v3"; | ||
option java_outer_classname = "HttpServiceProto"; | ||
option java_multiple_files = true; | ||
option go_package = "github.com/envoyproxy/go-control-plane/envoy/config/core/v3;corev3"; | ||
option (udpa.annotations.file_status).package_version_status = ACTIVE; | ||
|
||
// [#protodoc-title: HTTP services] | ||
|
||
// HTTP service configuration. | ||
message HttpService { | ||
// The service's HTTP URI. For example: | ||
// | ||
// .. code-block:: yaml | ||
// | ||
// http_uri: | ||
// uri: https://www.myserviceapi.com/v1/data | ||
// cluster: www.myserviceapi.com|443 | ||
// | ||
HttpUri http_uri = 1; | ||
|
||
// Specifies a list of HTTP headers that should be added to each request | ||
// handled by this virtual host. | ||
repeated HeaderValueOption request_headers_to_add = 2 | ||
[(validate.rules).repeated = {max_items: 1000}]; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
source/extensions/tracers/opentelemetry/http_trace_exporter.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
#include "source/extensions/tracers/opentelemetry/http_trace_exporter.h" | ||
|
||
#include <chrono> | ||
#include <memory> | ||
#include <string> | ||
#include <vector> | ||
|
||
#include "source/common/common/enum_to_int.h" | ||
#include "source/common/common/logger.h" | ||
#include "source/common/protobuf/protobuf.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace Tracers { | ||
namespace OpenTelemetry { | ||
|
||
OpenTelemetryHttpTraceExporter::OpenTelemetryHttpTraceExporter( | ||
Upstream::ClusterManager& cluster_manager, | ||
const envoy::config::core::v3::HttpService& http_service) | ||
: cluster_manager_(cluster_manager), http_service_(http_service) { | ||
|
||
// Prepare and store headers to be used later on each export request | ||
for (const auto& header_value_option : http_service_.request_headers_to_add()) { | ||
parsed_headers_to_add_.push_back({Http::LowerCaseString(header_value_option.header().key()), | ||
header_value_option.header().value()}); | ||
} | ||
} | ||
|
||
joaopgrassi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
bool OpenTelemetryHttpTraceExporter::log(const ExportTraceServiceRequest& request) { | ||
std::string request_body; | ||
|
||
const auto ok = request.SerializeToString(&request_body); | ||
if (!ok) { | ||
joaopgrassi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ENVOY_LOG(warn, "Error while serializing the binary proto ExportTraceServiceRequest."); | ||
return false; | ||
} | ||
|
||
const auto thread_local_cluster = | ||
joaopgrassi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
cluster_manager_.getThreadLocalCluster(http_service_.http_uri().cluster()); | ||
if (thread_local_cluster == nullptr) { | ||
ENVOY_LOG(error, "OTLP HTTP exporter failed: [cluster = {}] is not configured", | ||
http_service_.http_uri().cluster()); | ||
return false; | ||
} | ||
|
||
Http::RequestMessagePtr message = Http::Utility::prepareHeaders(http_service_.http_uri()); | ||
|
||
// The request follows the OTLP HTTP specification: | ||
// https://github.com/open-telemetry/opentelemetry-proto/blob/v1.0.0/docs/specification.md#otlphttp. | ||
message->headers().setReferenceMethod(Http::Headers::get().MethodValues.Post); | ||
joaopgrassi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
message->headers().setReferenceContentType(Http::Headers::get().ContentTypeValues.Protobuf); | ||
|
||
// Add all custom headers to the request. | ||
for (const auto& header_pair : parsed_headers_to_add_) { | ||
message->headers().setReference(header_pair.first, header_pair.second); | ||
} | ||
message->body().add(request_body); | ||
|
||
const auto options = Http::AsyncClient::RequestOptions().setTimeout(std::chrono::milliseconds( | ||
DurationUtil::durationToMilliseconds(http_service_.http_uri().timeout()))); | ||
|
||
Http::AsyncClient::Request* in_flight_request = | ||
thread_local_cluster->httpAsyncClient().send(std::move(message), *this, options); | ||
|
||
if (in_flight_request == nullptr) { | ||
return false; | ||
} | ||
|
||
active_requests_.add(*in_flight_request); | ||
return true; | ||
} | ||
|
||
void OpenTelemetryHttpTraceExporter::onSuccess(const Http::AsyncClient::Request& request, | ||
Http::ResponseMessagePtr&& http_response) { | ||
active_requests_.remove(request); | ||
const auto response_code = Http::Utility::getResponseStatus(http_response->headers()); | ||
if (response_code != enumToInt(Http::Code::OK)) { | ||
ENVOY_LOG(error, | ||
"OTLP HTTP exporter received a non-success status code: {} while exporting the OTLP " | ||
"message", | ||
response_code); | ||
} | ||
} | ||
|
||
void OpenTelemetryHttpTraceExporter::onFailure(const Http::AsyncClient::Request& request, | ||
Http::AsyncClient::FailureReason reason) { | ||
active_requests_.remove(request); | ||
ENVOY_LOG(debug, "The OTLP export request failed. Reason {}", enumToInt(reason)); | ||
} | ||
|
||
} // namespace OpenTelemetry | ||
} // namespace Tracers | ||
} // namespace Extensions | ||
} // namespace Envoy |
48 changes: 48 additions & 0 deletions
48
source/extensions/tracers/opentelemetry/http_trace_exporter.h
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
#pragma once | ||
|
||
#include "envoy/config/core/v3/http_service.pb.h" | ||
#include "envoy/upstream/cluster_manager.h" | ||
|
||
#include "source/common/common/logger.h" | ||
#include "source/common/http/async_client_impl.h" | ||
#include "source/common/http/async_client_utility.h" | ||
#include "source/common/http/headers.h" | ||
#include "source/common/http/message_impl.h" | ||
#include "source/common/http/utility.h" | ||
#include "source/extensions/tracers/opentelemetry/trace_exporter.h" | ||
|
||
#include "opentelemetry/proto/collector/trace/v1/trace_service.pb.h" | ||
|
||
namespace Envoy { | ||
namespace Extensions { | ||
namespace Tracers { | ||
namespace OpenTelemetry { | ||
|
||
/** | ||
* Exporter for OTLP traces over HTTP. | ||
*/ | ||
class OpenTelemetryHttpTraceExporter : public OpenTelemetryTraceExporter, | ||
public Http::AsyncClient::Callbacks { | ||
public: | ||
OpenTelemetryHttpTraceExporter(Upstream::ClusterManager& cluster_manager, | ||
const envoy::config::core::v3::HttpService& http_service); | ||
|
||
bool log(const ExportTraceServiceRequest& request) override; | ||
|
||
// Http::AsyncClient::Callbacks. | ||
void onSuccess(const Http::AsyncClient::Request&, Http::ResponseMessagePtr&&) override; | ||
void onFailure(const Http::AsyncClient::Request&, Http::AsyncClient::FailureReason) override; | ||
void onBeforeFinalizeUpstreamSpan(Tracing::Span&, const Http::ResponseHeaderMap*) override {} | ||
|
||
private: | ||
Upstream::ClusterManager& cluster_manager_; | ||
envoy::config::core::v3::HttpService http_service_; | ||
// Track active HTTP requests to be able to cancel them on destruction. | ||
Http::AsyncClientRequestTracker active_requests_; | ||
std::vector<std::pair<const Http::LowerCaseString, const std::string>> parsed_headers_to_add_; | ||
}; | ||
|
||
} // namespace OpenTelemetry | ||
} // namespace Tracers | ||
} // namespace Extensions | ||
} // namespace Envoy |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 changes: 1 addition & 1 deletion
2
source/extensions/tracers/opentelemetry/span_context_extractor.cc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you model this on ext_authz
HttpService
? I notice there are a few subtle differences, e.g. use ofHttpUri
there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I didn't model based on that. I'm not sure, but I don't see a point in using the
HttpUri
type:It forces defining a URI in the config, which won't be used in the OTLP exporter. In this PR, the exporter obtains an HTTP client from the thread local cluster which already has the base url configured. This was something that I noticed in @AlexanderEllis original PR and commented about it #28454 (comment). In the old PR, I could put whatever I wanted in
uri:
and it didn't matter.I looked, the
ext_authz
seems to be doing the same https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc#L254 and I also don't see any usage of theUri
there. It only access the cluster name, to get the thread local cluster and also get the HTTP client.And by looking at this example https://github.com/envoyproxy/envoy/blob/main/examples/ext_authz/config/http-service.yaml#L32, unless I'm missing something, I don't see where
ext_authz
is coming from (used inuri
), which confirms that it's just there to satisfy the field inHttpUri
being required.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the question I'd have is "the next time we do something similar to what ext_authz did, can we use HttpService for this?". FWIW this is not theoretical, we plan to do this for ext_proc at some point I think. @adisuissa do you have thoughts on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting… it took me a while to dig through the history of
HttpUri
, and found this comment thread interesting in particular while trying to understand what was the motivation behindHttpUri.uri
.Seems it was introduced to set the path, scheme and other parts of the request, while
HttpUri.cluster
sets the used Envoy cluster.The 2 places I saw it is used as intended seem to be:
While the rest seem to be using just the
cluster
part.I agree that ext-authz seems to be using it just for the
cluster
, and disregards theuri
, but maybe it should have used it as intended (maybe in the future?).I think it will be better to use
HttpUri
as is and probably try to reuse some of the code from jwks fetcher for this use-case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adisuissa I changed it to use
HttpUri
following the jwks fetcher example you gave 3be83f8. Looks a bit simpler now :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't
request_headers_to_add
a generic thing for an HTTP service? Seems a standard thing you might want to do when invoking an HTTP service, e.g. to set an auth token.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From HTTP perspective, yes
request_headers_to_add
is generic thing.However, what I meant is from user and practical perspective, this won't necessarily be achieved by this field in base proto here. More specifically:
In short, I don't have strong opinion here and I am just not sure about practical value of this
request_headers_to_add
field, as it can be easily achieved by extension specific config (and in a flexible way). This field here is probably good for simple add-header needs though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel
request_headers_to_add
here make sense for "static" header values. If there's a need for cases where a header value is not know in advance, like the client_id example, those can still be added "on the fly". I think one does not negate the other.The GrpcService also has a headers property, so the goal was to be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think for some simple use cases, like service specific static auth tokens, this is still reasonable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I agree with its value for simple use case.
Thanks all for discussion.