Skip to content

Commit

Permalink
Revert "Change OTLP HTTP content_type default to binary (#2558)" (#2562)
Browse files Browse the repository at this point in the history 
This reverts commit a883dc7.
  • Loading branch information
@lalitb
lalitb authored Feb 27, 2024
1 parent a883dc7 commit 719c60f
Show file tree
Hide file tree
Showing 14 changed files with 6 additions and 154 deletions.
3 changes: 0 additions & 3 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ Increment the:

## [Unreleased]

* [SDK] Change OTLP HTTP content_type default to binary
[#2558](https://github.com/open-telemetry/opentelemetry-cpp/pull/2558)

## [1.14.1] 2024-02-23

* [SDK] Restore Recordable API compatibility with versions < 1.14.0
Expand Down
1 change: 0 additions & 1 deletion exporters/otlp/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ cc_library(
cc_library(
name = "otlp_http_client",
srcs = [
"src/otlp_http.cc",
"src/otlp_http_client.cc",
],
hdrs = [
Expand Down
3 changes: 1 addition & 2 deletions exporters/otlp/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ if(WITH_OTLP_GRPC)
endif()

if(WITH_OTLP_HTTP)
add_library(opentelemetry_exporter_otlp_http_client src/otlp_http.cc
src/otlp_http_client.cc)
add_library(opentelemetry_exporter_otlp_http_client src/otlp_http_client.cc)
set_target_properties(opentelemetry_exporter_otlp_http_client
PROPERTIES EXPORT_NAME otlp_http_client)
set_target_version(opentelemetry_exporter_otlp_http_client)
Expand Down
4 changes: 0 additions & 4 deletions exporters/otlp/include/opentelemetry/exporters/otlp/otlp_environment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,6 @@ std::string GetOtlpDefaultHttpTracesEndpoint();
std::string GetOtlpDefaultHttpMetricsEndpoint();
std::string GetOtlpDefaultHttpLogsEndpoint();

std::string GetOtlpDefaultHttpTracesProtocol();
std::string GetOtlpDefaultHttpMetricsProtocol();
std::string GetOtlpDefaultHttpLogsProtocol();

// Compatibility with OTELCPP 1.8.2
inline std::string GetOtlpDefaultHttpEndpoint()
{
Expand Down
5 changes: 0 additions & 5 deletions exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@

#pragma once

#include "opentelemetry/common/macros.h"
#include "opentelemetry/nostd/string_view.h"
#include "opentelemetry/sdk/version/version.h"

OPENTELEMETRY_BEGIN_NAMESPACE
Expand All @@ -26,9 +24,6 @@ enum class HttpRequestContentType
kBinary,
};

OPENTELEMETRY_EXPORT HttpRequestContentType
GetOtlpHttpProtocolFromString(nostd::string_view name) noexcept;

} // namespace otlp
} // namespace exporter
OPENTELEMETRY_END_NAMESPACE
4 changes: 2 additions & 2 deletions exporters/otlp/include/opentelemetry/exporters/otlp/otlp_http_client.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ struct OtlpHttpClientOptions
/** SSL options. */
ext::http::client::HttpSslOptions ssl_options;

// By default, post binary data
HttpRequestContentType content_type = HttpRequestContentType::kBinary;
// By default, post json data
HttpRequestContentType content_type = HttpRequestContentType::kJson;

// If convert bytes into hex. By default, we will convert all bytes but id into base64
// This option is ignored if content_type is not kJson
Expand Down
72 changes: 0 additions & 72 deletions exporters/otlp/src/otlp_environment.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,78 +207,6 @@ std::string GetOtlpDefaultHttpLogsEndpoint()
return kDefault;
}

std::string GetOtlpDefaultHttpTracesProtocol()
{
constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_TRACES_PROTOCOL";
constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_PROTOCOL";
constexpr char kDefault[] = "http/protobuf";

std::string value;
bool exists;

exists = sdk_common::GetStringEnvironmentVariable(kSignalEnv, value);
if (exists)
{
return value;
}

exists = sdk_common::GetStringEnvironmentVariable(kGenericEnv, value);
if (exists)
{
return value;
}

return kDefault;
}

std::string GetOtlpDefaultHttpMetricsProtocol()
{
constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_METRICS_PROTOCOL";
constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_PROTOCOL";
constexpr char kDefault[] = "http/protobuf";

std::string value;
bool exists;

exists = sdk_common::GetStringEnvironmentVariable(kSignalEnv, value);
if (exists)
{
return value;
}

exists = sdk_common::GetStringEnvironmentVariable(kGenericEnv, value);
if (exists)
{
return value;
}

return kDefault;
}

std::string GetOtlpDefaultHttpLogsProtocol()
{
constexpr char kSignalEnv[] = "OTEL_EXPORTER_OTLP_LOGS_PROTOCOL";
constexpr char kGenericEnv[] = "OTEL_EXPORTER_OTLP_PROTOCOL";
constexpr char kDefault[] = "http/protobuf";

std::string value;
bool exists;

exists = sdk_common::GetStringEnvironmentVariable(kSignalEnv, value);
if (exists)
{
return value;
}

exists = sdk_common::GetStringEnvironmentVariable(kGenericEnv, value);
if (exists)
{
return value;
}

return kDefault;
}

bool GetOtlpDefaultGrpcTracesIsInsecure()
{
std::string endpoint = GetOtlpDefaultGrpcTracesEndpoint();
Expand Down
26 changes: 0 additions & 26 deletions exporters/otlp/src/otlp_http.cc
View file

This file was deleted.

2 changes: 1 addition & 1 deletion exporters/otlp/src/otlp_http_exporter_options.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace otlp
OtlpHttpExporterOptions::OtlpHttpExporterOptions()
{
url = GetOtlpDefaultHttpTracesEndpoint();
content_type = GetOtlpHttpProtocolFromString(GetOtlpDefaultHttpTracesProtocol());
content_type = HttpRequestContentType::kJson;
json_bytes_mapping = JsonBytesMappingKind::kHexId;
use_json_name = false;
console_debug = false;
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/src/otlp_http_log_record_exporter_options.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace otlp
OtlpHttpLogRecordExporterOptions::OtlpHttpLogRecordExporterOptions()
{
url = GetOtlpDefaultHttpLogsEndpoint();
content_type = GetOtlpHttpProtocolFromString(GetOtlpDefaultHttpLogsProtocol());
content_type = HttpRequestContentType::kJson;
json_bytes_mapping = JsonBytesMappingKind::kHexId;
use_json_name = false;
console_debug = false;
Expand Down
2 changes: 1 addition & 1 deletion exporters/otlp/src/otlp_http_metric_exporter_options.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace otlp
OtlpHttpMetricExporterOptions::OtlpHttpMetricExporterOptions()
{
url = GetOtlpDefaultMetricsEndpoint();
content_type = GetOtlpHttpProtocolFromString(GetOtlpDefaultHttpMetricsProtocol());
content_type = HttpRequestContentType::kJson;
json_bytes_mapping = JsonBytesMappingKind::kHexId;
use_json_name = false;
console_debug = false;
Expand Down
12 changes: 0 additions & 12 deletions exporters/otlp/test/otlp_http_exporter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -522,12 +522,6 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigJsonBytesMappingTest)
EXPECT_EQ(GetOptions(exporter).json_bytes_mapping, JsonBytesMappingKind::kHex);
}

TEST(OtlpHttpExporterTest, ConfigDefaultProtocolTest)
{
OtlpHttpExporterOptions opts;
EXPECT_EQ(opts.content_type, HttpRequestContentType::kBinary);
}

# ifndef NO_GETENV
// Test exporter configuration options with use_ssl_credentials
TEST_F(OtlpHttpExporterTestPeer, ConfigFromEnv)
Expand All @@ -537,7 +531,6 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigFromEnv)
setenv("OTEL_EXPORTER_OTLP_TIMEOUT", "20s", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_TRACES_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpExporter> exporter(new OtlpHttpExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -564,13 +557,11 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigFromEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_TRACES_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_PROTOCOL");
}

TEST_F(OtlpHttpExporterTestPeer, ConfigFromTracesEnv)
Expand All @@ -580,7 +571,6 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigFromTracesEnv)
setenv("OTEL_EXPORTER_OTLP_TRACES_TIMEOUT", "1eternity", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_TRACES_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_TRACES_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpExporter> exporter(new OtlpHttpExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -607,13 +597,11 @@ TEST_F(OtlpHttpExporterTestPeer, ConfigFromTracesEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_TRACES_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_TRACES_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_TRACES_PROTOCOL");
}
# endif

Expand Down
12 changes: 0 additions & 12 deletions exporters/otlp/test/otlp_http_log_record_exporter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,12 +642,6 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigJsonBytesMappingTest)
EXPECT_EQ(GetOptions(exporter).json_bytes_mapping, JsonBytesMappingKind::kHex);
}

TEST(OtlpHttpLogRecordExporterTest, ConfigDefaultProtocolTest)
{
OtlpHttpLogRecordExporterOptions opts;
EXPECT_EQ(opts.content_type, HttpRequestContentType::kBinary);
}

# ifndef NO_GETENV
// Test exporter configuration options with use_ssl_credentials
TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromEnv)
Expand All @@ -657,7 +651,6 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromEnv)
setenv("OTEL_EXPORTER_OTLP_TIMEOUT", "20s", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_LOGS_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpLogRecordExporter> exporter(new OtlpHttpLogRecordExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -684,13 +677,11 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_LOGS_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_PROTOCOL");
}

TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromLogsEnv)
Expand All @@ -700,7 +691,6 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromLogsEnv)
setenv("OTEL_EXPORTER_OTLP_LOGS_TIMEOUT", "20s", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_LOGS_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_LOGS_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpLogRecordExporter> exporter(new OtlpHttpLogRecordExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -727,13 +717,11 @@ TEST_F(OtlpHttpLogRecordExporterTestPeer, ConfigFromLogsEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_LOGS_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_LOGS_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_LOGS_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_LOGS_PROTOCOL");
}

TEST_F(OtlpHttpLogRecordExporterTestPeer, DefaultEndpoint)
Expand Down
12 changes: 0 additions & 12 deletions exporters/otlp/test/otlp_http_metric_exporter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -862,12 +862,6 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigJsonBytesMappingTest)
google::protobuf::ShutdownProtobufLibrary();
}

TEST(OtlpHttpMetricExporterTest, ConfigDefaultProtocolTest)
{
OtlpHttpMetricExporterOptions opts;
EXPECT_EQ(opts.content_type, HttpRequestContentType::kBinary);
}

#ifndef NO_GETENV
// Test exporter configuration options with use_ssl_credentials
TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromEnv)
Expand All @@ -877,7 +871,6 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromEnv)
setenv("OTEL_EXPORTER_OTLP_TIMEOUT", "20s", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_METRICS_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpMetricExporter> exporter(new OtlpHttpMetricExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -904,13 +897,11 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_METRICS_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_PROTOCOL");
}

TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromMetricsEnv)
Expand All @@ -920,7 +911,6 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromMetricsEnv)
setenv("OTEL_EXPORTER_OTLP_METRICS_TIMEOUT", "20s", 1);
setenv("OTEL_EXPORTER_OTLP_HEADERS", "k1=v1,k2=v2", 1);
setenv("OTEL_EXPORTER_OTLP_METRICS_HEADERS", "k1=v3,k1=v4", 1);
setenv("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL", "http/json", 1);

std::unique_ptr<OtlpHttpMetricExporter> exporter(new OtlpHttpMetricExporter());
EXPECT_EQ(GetOptions(exporter).url, url);
Expand All @@ -947,13 +937,11 @@ TEST_F(OtlpHttpMetricExporterTestPeer, ConfigFromMetricsEnv)
++range.first;
EXPECT_TRUE(range.first == range.second);
}
EXPECT_EQ(GetOptions(exporter).content_type, HttpRequestContentType::kJson);

unsetenv("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT");
unsetenv("OTEL_EXPORTER_OTLP_METRICS_TIMEOUT");
unsetenv("OTEL_EXPORTER_OTLP_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_METRICS_HEADERS");
unsetenv("OTEL_EXPORTER_OTLP_METRICS_PROTOCOL");
}

TEST_F(OtlpHttpMetricExporterTestPeer, DefaultEndpoint)
Expand Down

2 comments on commit 719c60f

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp sdk Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 719c60f Previous: a883dc7 Ratio
BM_SpanCreation 1044.970501790654 ns/iter 501.1267058143509 ns/iter 2.09
BM_NoopSpanCreation 353.8611089673056 ns/iter 125.38010430827558 ns/iter 2.82

This comment was automatically generated by workflow using github-action-benchmark.

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'OpenTelemetry-cpp api Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 719c60f Previous: a883dc7 Ratio
BM_SpinLockThrashing/4/process_time/real_time 1.77974923182342 ms/iter 0.7732377468960957 ms/iter 2.30
BM_ProcYieldSpinLockThrashing/1/process_time/real_time 0.5293704740253665 ms/iter 0.15819931030273438 ms/iter 3.35

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.