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

Use tracing fields to store variable log data #134

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

NickCaplinger
Copy link
Contributor

Hey again!

This PR goes through all of the places that the crate uses tracing events and pushes all of the variable data into fields.

This is something that we recently did for our backend services, and it made it far easier to analyze our logs using Google's Log Analytics tool. We're also able to easily pull out data from these fields, which is helpful not only for Log Analytics, but for log-based metrics!

We actually run our services with debug-level logging enabled for the firestore crate in order to get better insight into the behavior and performance of the database, and track down cases of transaction contention, which is why I've made this PR.

The root-level documentation of tracing has additional explanation of the merits of using fields here.

Below is an example of what this looks like in Log Analytics for one of our services. This maybe isn't the best example because I've picked a small sample so I didn't have to draw so many black boxes, but you can see where some firestore events haven't been grouped.

log_analytics_example

Thank you for your continued, high quality work on this project!

This greatly assists when analyzing errors using a tool like
Google's Log Analytics.

See the root-level documentation of tracing for more explanation
 on the merits of doing this:
https://docs.rs/tracing/latest/tracing/#recording-fields
@abdolence
Copy link
Owner

Hey, first of all, you don't have to convince me that structured tracing/logging is great :)
As you can see I already implemented spans for tracing with some attributes, and logs was missing structured fields indeed.

When I started this crate it was actually using old log crate which didn't have an ability to propagate normally fields, and then I just haven't changed it yet, so I really appreciate your help in this area 🏆

Generally it looks good, but I need a bit more time to review when I have time, so I'll come back to you.

@abdolence
Copy link
Owner

PS Also it fails on tests/compilation in pipelines :)

@NickCaplinger
Copy link
Contributor Author

Hey, first of all, you don't have to convince me that structured tracing/logging is great :)
As you can see I already implemented spans for tracing with some attributes, and logs was missing structured fields indeed.

When I started this crate it was actually using old log crate which didn't have an ability to propagate normally fields, and then I just haven't changed it yet, so I really appreciate your help in this area 🏆

This is exactly what happened to us, so I completely understand!

Generally it looks good, but I need a bit more time to review when I have time, so I'll come back to you.

Absolutely! Given the nature of the PR, it's not exactly urgent.

Thanks again. :)

@abdolence
Copy link
Owner

Hey, it still doesn't compile for me :)

image

@abdolence
Copy link
Owner

I'm trying to test all the features enabled:

cargo test --all-features

@NickCaplinger
Copy link
Contributor Author

Yikes, sorry about that!

@abdolence
Copy link
Owner

In some places, it looks a bit now repetitive since we have tracing spans, etc:

Firestore Get Doc{/firestore/collection_name="integration-test-childs" /firestore/document_name="projects/latestbit/databases/(default)/documents/integration-nested-test/test-parent/integration-test-childs/test-child" /firestore/response_time=125}: firestore::db::get: Read document. document_path="projects/latestbit/databases/(default)/documents/integration-nested-test/test-parent/integration-test-childs/test-child" duration_milliseconds=125

I guess for some cases some people might want both, so I'll leave it for now.

I'm going to merge this, thanks for contributions! 👍🏻

@abdolence abdolence merged commit 50ae7ec into abdolence:master Oct 15, 2023
@NickCaplinger NickCaplinger deleted the features/use_tracing_fields branch October 15, 2023 20:29
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.

2 participants