You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/12/09 22:03:19 UTC
[GitHub] [arrow] lidavidm opened a new pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
lidavidm opened a new pull request #11925:
URL: https://github.com/apache/arrow/pull/11925
This adds two exporters that can be toggled with an environment variable, for debug use. One is the standard ostream exporter, which logs a human-friendly but machine-unfriendly format. The other uses a trick to log the JSON OTLP request format, which is easily parsable JSON but not very readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769759364
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
There is https://github.com/open-telemetry/opentelemetry-cpp/pull/1111 which would effectively accomplish the same thing. However if they do add it to the contrib repo that would involve some more packaging work for us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-995958816
@pitrou any other comments here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
pitrou commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-995991779
@github-actions crossbow submit -g cpp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769801773
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Interesting. Typically, the collector usage is deploying a container alongside the application, so without knowing anything else I'm not sure why it would be a lot more work.
@lidavidm and I went back and forth on the original PR about this, but I'm generally -1 on adding an exporter because exporters force a choice when used inside of a library. The Otel docs are pretty clear on how to use it within libraries (https://opentelemetry.io/docs/concepts/instrumenting-library/#opentelemetry-api)
Libraries should *just* instrument ideally, and testing can be done by constructing exporters in tests.
Including an exporter inside a library can also easily conflict with an applications N other exporters.
If these exporters are included I think they should be restricted to only when tests are being compiled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769760049
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -68,15 +112,14 @@ std::unique_ptr<sdktrace::SpanExporter> InitializeExporter() {
auto maybe_env_var = arrow::internal::GetEnvVar(kTracingBackendEnvVar);
if (maybe_env_var.ok()) {
auto env_var = maybe_env_var.ValueOrDie();
- if (env_var == "otlp_http") {
-#ifdef ARROW_WITH_OPENTELEMETRY
+ if (env_var == "ostream") {
+ return arrow::internal::make_unique<otel::exporter::trace::OStreamSpanExporter>();
+ } else if (env_var == "otlp_http") {
namespace otlp = opentelemetry::exporter::otlp;
otlp::OtlpHttpExporterOptions opts;
return arrow::internal::make_unique<otlp::OtlpHttpExporter>(opts);
-#else
- ARROW_LOG(WARNING) << "Requested " << kTracingBackendEnvVar << "=" < < < < env_var
- " but Arrow was not built with ARROW_WITH_OPENTELEMETRY";
-#endif
+ } else if (env_var == "arrow_otlp_stdout") {
Review comment:
We could add it.
It stands for [OpenTeLemetry Protocol](https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769801773
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Interesting. Typically, the collector usage is deploying a container alongside the application, so without knowing anything else I'm not sure why it would be a lot more work.
@lidavidm and I went back and forth on the original PR about this, but I'm generally -1 on adding an exporter because exporters force a choice when used inside of a library.
Libraries should *just* instrument ideally, and testing can be done by constructing exporters in tests.
Including an exporter inside a library can also easily conflict with an applications N other exporters.
If these exporters are included I think they should be restricted to only when tests are being compiled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
ursabot commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-996086938
Benchmark runs are scheduled for baseline = 27724a5ed0debf6ce283e0e224eac38964eb68d5 and contender = 8a4d8127aae80b27afb35755d44a8b61d770a706. 8a4d8127aae80b27afb35755d44a8b61d770a706 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Scheduled] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/612d3290f50b495a997101c4b5aef0fc...db504aa871de4ed9bdc9751ec5439404/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7f348081463b4749b3b9f075ae03eca7...973318c443e448bbafd32813dfd5e35c/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/b918c2af0f1c4abfbced295816505408...6421379fa9164cecaaa30b54663929ce/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-996086938
Benchmark runs are scheduled for baseline = 27724a5ed0debf6ce283e0e224eac38964eb68d5 and contender = 8a4d8127aae80b27afb35755d44a8b61d770a706. 8a4d8127aae80b27afb35755d44a8b61d770a706 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/612d3290f50b495a997101c4b5aef0fc...db504aa871de4ed9bdc9751ec5439404/)
[Finished :arrow_down:0.0% :arrow_up:0.9%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7f348081463b4749b3b9f075ae03eca7...973318c443e448bbafd32813dfd5e35c/)
[Finished :arrow_down:0.93% :arrow_up:0.4%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/b918c2af0f1c4abfbced295816505408...6421379fa9164cecaaa30b54663929ce/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769837872
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Also see: https://github.com/open-telemetry/community/discussions/734
Yeah, in-process propagation is gross, the original PR used to have an example of that (to instrument Flight in Python) but I dropped it in the name of slimming the PR down.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769803642
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Not totally understanding why a custom JSON format is less risky than using an upstream component or just having some documentation showing how to use the collector
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-996086938
Benchmark runs are scheduled for baseline = 27724a5ed0debf6ce283e0e224eac38964eb68d5 and contender = 8a4d8127aae80b27afb35755d44a8b61d770a706. 8a4d8127aae80b27afb35755d44a8b61d770a706 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/612d3290f50b495a997101c4b5aef0fc...db504aa871de4ed9bdc9751ec5439404/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7f348081463b4749b3b9f075ae03eca7...973318c443e448bbafd32813dfd5e35c/)
[Scheduled] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/b918c2af0f1c4abfbced295816505408...6421379fa9164cecaaa30b54663929ce/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-995194743
This LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-995992817
Revision: f7cbd4d215ce83a149e461db2a3d5f0dce6532ec
Submitted crossbow builds: [ursacomputing/crossbow @ actions-1317](https://github.com/ursacomputing/crossbow/branches/all?query=actions-1317)
|Task|Status|
|----|------|
|test-build-cpp-fuzz|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-build-cpp-fuzz)|
|test-conda-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-conda-cpp)|
|test-conda-cpp-valgrind|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-1317-azure-test-conda-cpp-valgrind)|
|test-debian-10-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-debian-10-cpp-amd64)|
|test-debian-10-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-debian-10-cpp-i386)|
|test-debian-11-cpp-amd64|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-debian-11-cpp-amd64)|
|test-debian-11-cpp-i386|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-debian-11-cpp-i386)|
|test-fedora-33-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-fedora-33-cpp)|
|test-ubuntu-18.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-18.04-cpp)|
|test-ubuntu-18.04-cpp-release|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-18.04-cpp-release)|
|test-ubuntu-18.04-cpp-static|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-18.04-cpp-static)|
|test-ubuntu-20.04-cpp|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-20.04-cpp)|
|test-ubuntu-20.04-cpp-14|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-20.04-cpp-14)|
|test-ubuntu-20.04-cpp-17|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-20.04-cpp-17)|
|test-ubuntu-20.04-cpp-bundled|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-20.04-cpp-bundled)|
|test-ubuntu-20.04-cpp-thread-sanitizer|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-1317-github-test-ubuntu-20.04-cpp-thread-sanitizer)|
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou closed pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
pitrou closed pull request #11925:
URL: https://github.com/apache/arrow/pull/11925
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-994972308
> The other uses a trick to log the JSON OTLP request format, which is easily parsable JSON but not very readable.
Can't we just output the machine-readable JSON and users can use `jq` or something? No reason to write code for tools that already do an amazing job of formatting JSON
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769744355
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -68,15 +112,14 @@ std::unique_ptr<sdktrace::SpanExporter> InitializeExporter() {
auto maybe_env_var = arrow::internal::GetEnvVar(kTracingBackendEnvVar);
if (maybe_env_var.ok()) {
auto env_var = maybe_env_var.ValueOrDie();
- if (env_var == "otlp_http") {
-#ifdef ARROW_WITH_OPENTELEMETRY
+ if (env_var == "ostream") {
+ return arrow::internal::make_unique<otel::exporter::trace::OStreamSpanExporter>();
+ } else if (env_var == "otlp_http") {
namespace otlp = opentelemetry::exporter::otlp;
otlp::OtlpHttpExporterOptions opts;
return arrow::internal::make_unique<otlp::OtlpHttpExporter>(opts);
-#else
- ARROW_LOG(WARNING) << "Requested " << kTracingBackendEnvVar << "=" < < < < env_var
- " but Arrow was not built with ARROW_WITH_OPENTELEMETRY";
-#endif
+ } else if (env_var == "arrow_otlp_stdout") {
Review comment:
Should there be, similarly, "arrow_oltp_stderr"?
(btw, what does "oltp" mean?)
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -68,15 +112,14 @@ std::unique_ptr<sdktrace::SpanExporter> InitializeExporter() {
auto maybe_env_var = arrow::internal::GetEnvVar(kTracingBackendEnvVar);
if (maybe_env_var.ok()) {
auto env_var = maybe_env_var.ValueOrDie();
- if (env_var == "otlp_http") {
-#ifdef ARROW_WITH_OPENTELEMETRY
+ if (env_var == "ostream") {
+ return arrow::internal::make_unique<otel::exporter::trace::OStreamSpanExporter>();
+ } else if (env_var == "otlp_http") {
namespace otlp = opentelemetry::exporter::otlp;
otlp::OtlpHttpExporterOptions opts;
return arrow::internal::make_unique<otlp::OtlpHttpExporter>(opts);
-#else
- ARROW_LOG(WARNING) << "Requested " << kTracingBackendEnvVar << "=" < < < < env_var
- " but Arrow was not built with ARROW_WITH_OPENTELEMETRY";
-#endif
+ } else if (env_var == "arrow_otlp_stdout") {
Review comment:
Should there be, similarly, "arrow_otlp_stderr"?
(btw, what does "otlp" mean?)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769742814
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Is there a feature request on the OpenTelemetry side for them to expose this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769812488
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
> Not totally understanding why a custom JSON format is less risky than using an upstream component or just having some documentation showing how to use the collector
Sorry, I meant just in terms of upstream changing APIs on us or something like that. The approach taken here does use the upstream generated Protobuf code and they might hide it from the public headers in the future, for instance.
> Libraries should just instrument ideally, and testing can be done by constructing exporters in tests.
That is the intent, but I think Arrow occupies a halfway point between library and application, especially when bindings come into play. You can't enable C++ exporters from Python or R, for instance.
The exporters are not configured by default, for what it's worth - but they can be enabled by env var. The intent is that for development or testing, the env var can be used to easily dump spans somewhere, but an application would not use the env var and would configure its own exporter/tracer provider.
That said, this would all mostly be moot if Conbench ran a collector alongside test runs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot edited a comment on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
ursabot edited a comment on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-996086938
Benchmark runs are scheduled for baseline = 27724a5ed0debf6ce283e0e224eac38964eb68d5 and contender = 8a4d8127aae80b27afb35755d44a8b61d770a706. 8a4d8127aae80b27afb35755d44a8b61d770a706 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/612d3290f50b495a997101c4b5aef0fc...db504aa871de4ed9bdc9751ec5439404/)
[Scheduled] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/7f348081463b4749b3b9f075ae03eca7...973318c443e448bbafd32813dfd5e35c/)
[Finished :arrow_down:0.93% :arrow_up:0.4%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/b918c2af0f1c4abfbced295816505408...6421379fa9164cecaaa30b54663929ce/)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python. Runs only benchmarks with cloud = True
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769801773
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Interesting. Typically, the collector usage is deploying a container alongside the application, so without knowing anything else I'm not sure why it would be a lot more work.
@lidavidm and I went back and forth on the original PR about this, but I'm generally -1 on adding an exporter because exporters force a choice when used inside of a library. The Otel docs are pretty clear on how to use it within libraries (https://opentelemetry.io/docs/concepts/instrumenting-library/#opentelemetry-api)
Libraries should *just* instrument ideally, and testing can be done by constructing exporters in tests.
Including an exporter inside a library can also easily conflict with an application's N other exporters.
If these exporters are included I think they should be restricted to only when tests are being compiled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-990325579
Example of ostream output:
```
{
name : test
trace_id : 577bb5359fd4c9babdfb7e8def43d476
span_id : 41655856137c4635
tracestate :
parent_span_id: 0000000000000000
start : 1639087427620946845
duration : 8538876
description :
span kind : Internal
status : Unset
attributes :
thread_id: 140062552533184
foo: bar
events :
links :
resources :
service.name: unknown_service
telemetry.sdk.version: 1.1.0
telemetry.sdk.name: opentelemetry
telemetry.sdk.language: cpp
instr-lib : arrow
}
```
Example of OTLP output:
```
{"resource":{"attributes":[{"key":"service.name","value":{"stringValue":"unknown_service"}},{"key":"telemetry.sdk.version","value":{"stringValue":"1.1.0"}},{"key":"telemetry.sdk.name","value":{"stringValue":"opentelemetry"}},{"key":"telemetry.sdk.language","value":{"stringValue":"cpp"}}]},"instrumentationLibrarySpans":[{"instrumentationLibrary":{"name":"arrow"},"spans":[{"traceId":"ID+DVD1Rarxdh8dNLchUmw==","spanId":"7IGA2C88AQk=","parentSpanId":"AAAAAAAAAAA=","name":"test","kind":"SPAN_KIND_INTERNAL","startTimeUnixNano":"1639087084846448841","endTimeUnixNano":"1639087084849284338","attributes":[{"key":"foo","value":{"stringValue":"bar"}},{"key":"thread_id","value":{"stringValue":"140253881153728"}}]},{"traceId":"pPN+j22d17tQVbHZpG6pbA==","spanId":"6apExpYokXw=","parentSpanId":"AAAAAAAAAAA=","name":"test","kind":"SPAN_KIND_INTERNAL","startTimeUnixNano":"1639087084850354448","endTimeUnixNano":"1639087084853673659","attributes":[{"key":"foo","value":{"stringValue":"bar"}},{"key":"th
read_id","value":{"stringValue":"140253881153728"}}]},{"traceId":"yVWEeB/z4NedBKrCdezTMg==","spanId":"4AGb3ndTHek=","parentSpanId":"AAAAAAAAAAA=","name":"test","kind":"SPAN_KIND_INTERNAL","startTimeUnixNano":"1639087084873028556","endTimeUnixNano":"1639087084876243603","attributes":[{"key":"foo","value":{"stringValue":"bar"}},{"key":"thread_id","value":{"stringValue":"140253881153728"}}]}]}]}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-990324816
https://issues.apache.org/jira/browse/ARROW-15044
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] cpcloud commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
cpcloud commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769820941
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
> You can't enable C++ exporters from Python or R, for instance.
/me grumbles. I forgot about that. Ugh, I guess that means you'd probably have to use context propagation in-process which is kind of gross.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769776814
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
Sounds good. CC @cpcloud any thoughts? There was such an exporter on the original PR (https://github.com/apache/arrow/pull/10260#discussion_r736884015) but as I recall the recommendation was to use the OTLP collector instead, so it was dropped. However, it seems the collector would be a lot more work for Conbench to integrate + is less convenient for local development workflows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769763751
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
The least riskiest thing would be to just define our own JSON format and not try to reuse any upstream components.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#issuecomment-994976717
I'm not planning on writing any code to format the OTLP output further. I just added that to contrast it with the ostream exporter. (That said, the ostream exporter is not super useful once you log more than ~50 traces, so maybe it's not worth having at all.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a change in pull request #11925: ARROW-15044: [C++] Add OpenTelemetry exporters for debugging use
Posted by GitBox <gi...@apache.org>.
pitrou commented on a change in pull request #11925:
URL: https://github.com/apache/arrow/pull/11925#discussion_r769766824
##########
File path: cpp/src/arrow/util/tracing_internal.cc
##########
@@ -53,6 +60,43 @@ namespace {
namespace sdktrace = opentelemetry::sdk::trace;
+// Custom JSON stdout exporter. Leverages the OTLP HTTP exporter's
+// utilities to log the same format that would be sent to OTLP.
Review comment:
If that's not too much work that would sound reasonable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org