-
Notifications
You must be signed in to change notification settings - Fork 13
feat(logging): logging in libdatadog #1018
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
==========================================
- Coverage 70.88% 70.85% -0.04%
==========================================
Files 331 334 +3
Lines 49957 50481 +524
==========================================
+ Hits 35414 35769 +355
- Misses 14543 14712 +169
🚀 New features to boost your workflow:
|
BenchmarksComparisonBenchmark execution time: 2025-06-05 16:28:29 Comparing candidate commit 7d9f841 in PR branch Found 0 performance improvements and 2 performance regressions! Performance is the same for 50 metrics, 2 unstable metrics. scenario:benching string interning on wordpress profile
scenario:sql/obfuscate_sql_string
|
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
The symbol prefixes I know are already in use are:
Can we name this |
actually this threw me off a few times, currently we have all sort of prefixes depending on the context (crate name vs dir name vs rust type name vs ffi method names) do you mean anything specific like FFI methods or in general? I agree that atleast ffi methods should be |
There are conventions for the full name too, but they are less standardized. I was specifically talking about the FFI symbol prefixes with this comment. Within a package, you should be consistent with the FFI casing, types, etc, but across packages, I want to be consistent with And also I would prefer |
A lot of cognitive load... but anyway I would stick to what we have already. crate name: datadog-log and datadog-log-ffi |
e0e5c8a
to
4c05b2f
Compare
3621f13
to
3b0b46b
Compare
fb63772
to
bd9ab62
Compare
datadog-log/src/logger.rs
Outdated
.event_format(format) | ||
.with_writer(dynamic_writer.clone()); | ||
|
||
tracing_subscriber::registry() |
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.
What, if anything will happen when libdatadog crates are used in other crates, like dd-trace-rs
? I assume if the other crate registers a subscriber then it should just work? No need to use this logger crate?
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.
they must not call into logger, this is exclusively made for the hosted languages.
when comes to other rust libraries, they should follow the practice of allowing apps to setup the tracing subscriber.
TLDR; a rust library should never setup a global subscriber like this but here our use case is FFI based integrations which is fair usage.
bd9ab62
to
fc42932
Compare
af9b0d6
to
9c8bb42
Compare
059c87f
to
1c04b9e
Compare
1c04b9e
to
997bcf2
Compare
## Summary of changes Fixes https://datadoghq.atlassian.net/browse/APMSP-1835 This PR adds enabling the file based logging for libdatadog, ie we start to write the logs to the configured file as it happens. ## Reason for change This is missing feature in the native exporter and must have for troubleshooting. ## Implementation details This PR DataDog/libdatadog#1018 does the bulk of the logging setup. In this, we are only calling the exposed APIs. ## Test coverage Since all the coverage is done in libdatadog there nothing to test in CI, however I verified manually that log file is created and written. ## Other details <!-- Fixes #{issue} --> We will need DataDog/libdatadog#1088 for fully support the file based logging practices. Also, we will need a release of libdatadog to pass the CI. <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. --> --------- Co-authored-by: Andrew Lock <[email protected]> Co-authored-by: Steven Bouwkamp <[email protected]>
## Summary of changes Fixes https://datadoghq.atlassian.net/browse/APMSP-1835 This PR adds enabling the file based logging for libdatadog, ie we start to write the logs to the configured file as it happens. ## Reason for change This is missing feature in the native exporter and must have for troubleshooting. ## Implementation details This PR DataDog/libdatadog#1018 does the bulk of the logging setup. In this, we are only calling the exposed APIs. ## Test coverage Since all the coverage is done in libdatadog there nothing to test in CI, however I verified manually that log file is created and written. ## Other details <!-- Fixes #{issue} --> We will need DataDog/libdatadog#1088 for fully support the file based logging practices. Also, we will need a release of libdatadog to pass the CI. <!--⚠️ Note: where possible, please obtain 2 approvals prior to merging. Unless CODEOWNERS specifies otherwise, for external teams it is typically best to have one review from a team member, and one review from apm-dotnet. Trivial changes do not require 2 reviews. --> --------- Co-authored-by: Andrew Lock <[email protected]> Co-authored-by: Steven Bouwkamp <[email protected]>
What does this PR do?
This pull request includes several changes to add new logging functionality and update dependencies across multiple files. The most important changes include adding new
datadog-log
anddatadog-log-ffi
crates, updating dependencies to usetracing
instead oflog
.Check the RFC for more details included in the PR.
Done
Remaining work
Motivation
Fixes https://datadoghq.atlassian.net/browse/APMSP-1835
This is part of bigger effort to allow language integration access to the logs happening in the libdatadog.
Note
This is blocking issue for data-pipeline .NET integration.
Why separate crate?
Holistically speaking, this functionality not only works for data-pipeline but for any integration. Hence, I decided to move to its own crate just like telemetry. This also gives a place to start adding any other additional crate agnostic logging logic.
Additional Notes
How to test the change?