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

Add Support for Tracing #453

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Add Support for Tracing #453

merged 1 commit into from
Oct 15, 2024

Conversation

dillonrg
Copy link
Contributor

@dillonrg dillonrg commented Oct 1, 2024

Context
The tracing crate enables more robust instrumentation of Rust programs compared to basic logging. With this patch, we introduce the tracing crate into AKD and only enable it when the tracing feature is specified. By default, the feature is disabled and we continue to leverage basic logging.

In addition to adding tracing based logging and instrumentation throughout the akd lib (i.e. not akd_core), various grammatical and organizational improvements were made in areas where tracing was being added. Notably, the log_metrics functions which exist throughout the storage layer were updated to simply log with info level instead of taking an argument to specify the level. Rationale being that the log_metrics functions are only called when the runtime_metrics feature is enabled and info is a fair median to assume.

Testing
Since no major functional changes were made, the changes were tested via existing automated tests with various different feature flags being passed to toggle tracing on and off.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 1, 2024
@dillonrg dillonrg force-pushed the add-tracing branch 3 times, most recently from 5ac611f to 3f18b6b Compare October 2, 2024 00:21
@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.91%. Comparing base (3ce5335) to head (f5ce4e2).
Report is 24 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
- Coverage   88.61%   87.91%   -0.71%     
==========================================
  Files          39       38       -1     
  Lines        9109     8192     -917     
==========================================
- Hits         8072     7202     -870     
+ Misses       1037      990      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dillonrg dillonrg marked this pull request as ready for review October 2, 2024 07:15
Copy link
Contributor

@slawlor slawlor left a comment

Choose a reason for hiding this comment

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

Lgtm thanks for the great addition!

Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

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

LGTM!

@dillonrg dillonrg force-pushed the add-tracing branch 3 times, most recently from 8bd1527 to b40f08c Compare October 13, 2024 09:10
@dillonrg dillonrg requested review from kevinlewi and slawlor October 14, 2024 04:18
Copy link
Contributor

@kevinlewi kevinlewi left a comment

Choose a reason for hiding this comment

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

LGTM!

akd/src/lib.rs Outdated Show resolved Hide resolved
**Context**
The [`tracing`](https://docs.rs/tracing/latest/tracing/index.html) crate enables more robust instrumentation
of Rust programs compared to basic logging. With this patch, we introduce the `tracing` crate
into AKD and only enable it when the `tracing` feature is specified. By default,
the feature is disabled and we continue to leverage basic logging. To enable instrumentation,
we leverage another feature `tracing_instrument` to control whether or not AKD generates spans
for instrumented functions.

In addition to adding `tracing` based logging and instrumentation throughout the
`akd` lib (i.e. not `akd_core`), various grammatical and organizational improvements
were made in areas where tracing was being added. Notably, the `log_metrics` functions
which exist throughout the storage layer were updated to simply log with `info` level
instead of taking an argument to specify the level. Rationale being that the `log_metrics`
functions are only called when the `runtime_metrics` feature is enabled and `info` is
a fair median to assume.

**Testing**
Since no major functional changes were made, the changes were tested via existing
automated tests with various different feature flags being passed to toggle `tracing`
on and off.
@dillonrg dillonrg merged commit a38c9be into facebook:main Oct 15, 2024
14 checks passed
@dillonrg dillonrg deleted the add-tracing branch October 15, 2024 00:49
Copy link
Contributor

@slawlor slawlor left a comment

Choose a reason for hiding this comment

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

couple of tiny changes, but overall lgtm

pub(crate) async fn retrieve_azks(&self) -> Result<Azks, crate::errors::AkdError> {
Directory::<TC, S, V>::get_azks_from_storage(&self.storage, false).await
}

#[cfg_attr(feature = "tracing_instrument", tracing::instrument(skip_all, fields(ignore_cache = ignore_cache)))]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fields(ignore_cache)

/// Shim module to group logging-related macros more easily
/// when switching between [log](https://docs.rs/log/latest/log/)
/// and [tracing](https://docs.rs/tracing/latest/tracing/).
pub mod log {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pub(crate)

@slawlor
Copy link
Contributor

slawlor commented Oct 15, 2024

whoops totally missed that this was already merged, sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants