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

move to using tracing instead of log #1579

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Conversation

demoray
Copy link
Contributor

@demoray demoray commented Jan 16, 2024

In order to move towards supporting OpenTelemetry for distributed tracing, we need to move the underlying logging implementation to use tracing.

This does not start exposing spans or tracing state yet.

ref: https://azure.github.io/azure-sdk/general_implementation.html

In order to move towards supporting OpenTelemetry, we need to move the
underlying logging implementation to use `tracing`.

This does _not_ start exposing spans or tracing state yet.

ref: https://azure.github.io/azure-sdk/general_implementation.html
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

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

I saw quite a few files where a use tracing::trace was added but no obvious indication of use. If a use log::trace were removed (and in some places it was), that makes sense; however, it's puzzling to see one without the other. Were log::trace instances left behind? I expanded a few files and didn't see any, but it was a small sample. I'd just ask that you double check if not sure. I suppose as long as the log crate was removed from all manifests it shouldn't be possible to use.

@@ -17,7 +17,7 @@ async-trait = "0.1"
azure_core = { path = "../core", version = "0.19" }
time = "0.3.10"
futures = "0.3"
log = "0.4"
tracing = "0.1.40"
Copy link
Member

Choose a reason for hiding this comment

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

Not to block this PR, but wondering if we should switch to workspace-inherited dependencies to better manage these centrally like we do in Azure/azure-sdk-for-net and, I believe, some other languages that allow for it.

https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#inheriting-a-dependency-from-a-workspace

@@ -1,5 +1,6 @@
use super::AuthorizationToken;
use azure_core::{auth::Secret, base64};
use tracing::trace;
Copy link
Member

Choose a reason for hiding this comment

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

Is this even used that you could just add a use and not remove one to, say, log::trace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tagged you in a comment that shows the change that resulted this change.

Comment on lines -66 to -67
#[macro_use]
extern crate log;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths, this is what required the inclusion of tracing::trace and the like in a handful of places.

Rather than import all of the macros from the tracing crate implicitly, this PR move to explicitly using the intended macros in the intended locations.

@demoray demoray merged commit f473bc9 into Azure:main Jan 22, 2024
22 checks passed
@demoray demoray deleted the move-to-tracing branch January 22, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants