Skip to content
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

[C++] Add support for OpenTelemetry logging #39898

Closed
benibus opened this issue Feb 1, 2024 · 2 comments · Fixed by #39905
Closed

[C++] Add support for OpenTelemetry logging #39898

benibus opened this issue Feb 1, 2024 · 2 comments · Fixed by #39905

Comments

@benibus
Copy link
Collaborator

benibus commented Feb 1, 2024

Describe the enhancement requested

This request stems from some ongoing discussions on how to enhance Flight/FlightSQL's telemetry and improve users' debugging experience. I believe that utilizing OpenTelemetry's logging facilities would be a step in the right direction as the emitted logs can be associated with existing traces/spans.

A couple things to note:

  • The opentelemetry-cpp logging SDK was marked stable in v1.11.0, so we'd probably want to bump our version from 1.8.1.
  • I'm not necessarily suggesting that the OTel systems should integrate with ARROW_LOG and whatnot. I'd imagine we'd provide an independent API here.

FWIW I've done some experimentation using the (unstable) v1.8.1 SDK and I was happy with the results.

Component(s)

C++, FlightRPC

@jorisvandenbossche
Copy link
Member

For clarity, this issue is specifically for Flight? (as we already support OpenTelemetry in Arrow C++ in some places AFAIK?)

@benibus
Copy link
Collaborator Author

benibus commented Feb 8, 2024

Not necessarily just for Flight, although it would be most useful there. We do already support OpenTelemetry in Arrow, but we only really use it to create traces/spans, which indirectly serve as "logs" in their own way.

However, OTel (as of recently) provides additional tools for detailed logging within traces, where the emitted logs can be linked to the appropriate span_id/trace_id and exported along with everything else.

zeroshade pushed a commit that referenced this issue Jun 10, 2024
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on
how
to contribute here:
* [New Contributor's
Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing
Overview](https://arrow.apache.org/docs/dev/developers/overview.html)


If this is not a [minor
PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes).
Could you open an issue for this pull request on GitHub?
https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the
[Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.)
of the Apache Arrow project.

Then could you also rename the pull request title in the following
format?

    GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

    MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

    PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

-->

### Rationale for this change

Supporting OTel logs will help us improve diagnostics/debugging where
OTel tracing is utilized.

### What changes are included in this PR?

Primary changes:
- Bumps `opentelemetry-cpp` version to 1.13.0 to access the stable logs
SDK
- Introduces a new `ARROW_TELEMETRY` module for these additions (and in
anticipation of future OpenMetrics support)
- Integrates Otel logging facilities, provides `telemetry::Logger` class
that wraps an OTel logger and an API for creating loggers via OTel's
global logger provider
- Adds developer-friendly log record exporters, mimicking the
already-existing span exporters

Some auxiliary, but significant additions:
- Adds an extended/re-imagined version of the current `ArrowLog` APIs
that aims to be more flexible and configurable - which the OTel loggers
currently utilize. Notable details:
- Adds an abstract `util::Logger` class that enables creating custom
loggers.
- Adds a `LoggerRegistry` for global access to an arbitrary number of
loggers
- Adds new `ARROW_LOG_*` macros that take individual loggers as a
parameter. Additionally, they can be stripped at compile time based on a
minimum log level
- Definitely looking for opinions on this part of the PR,
specifically...
- Adds `ArrowLogLevel::ARROW_TRACE` as the lowest log level to mirror
the equivalent OTel enums
  
NOTE: I've added some log statements that utilize the OTel facilities to
Flight/FlightSQL - which are driven by the FlightSQL server tests. These
are for demonstrative purposes only and I intend to remove them prior to
merge

### Are these changes tested?

Yes (although the OTel-specific tests are currently light)

### Are there any user-facing changes?

This will introduce new APIs that are likely to be public.

<!--
If there are any breaking changes to public APIs, please uncomment the
line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->

<!--
Please uncomment the line below (and provide explanation) if the changes
fix either (a) a security vulnerability, (b) a bug that caused incorrect
or invalid data to be produced, or (c) a bug that causes a crash (even
when the API contract is upheld). We use this to highlight fixes to
issues that may affect users without their knowledge. For this reason,
fixing bugs that cause errors don't count, since those are usually
obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
* Closes: #39898
* GitHub Issue: #39898
@kou kou added this to the 17.0.0 milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants