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 filename at Error and Warn level in logs #665

Merged
merged 5 commits into from
Dec 7, 2023

Conversation

sauraank
Copy link
Contributor

@sauraank sauraank commented Dec 5, 2023

Description of change

Added filename (object key where filename was not available) at the error level and warn level logs since these are shown by default.

Used span to add the filename on the first occurence of filename in the callstack of the method. This results in logging like -

getattr{req=3 ino=34 name=“hello_new1.txt”}: mountpoint_s3::fuse: getattr failed: inode error: inode 34 (full key “hello_new1.txt”) for remote key “hello_new1.txt” is stale, replaced by inode 35 (full key “hello_new1.txt”)

In the PR #634 , it was getting logged as -

getattr{req=7 ino=2}: mountpoint_s3::fuse: getattr failed: inode error: inode 2 (full key “hello_new.txt”) for remote key “hello_new.txt” is stale, replaced by inode 3 (full key “hello_new.txt”)

Keeping both the PRs for now, in case we dont need the way it is done in this PR.

Also, added filename in all cases of InodeError as it was getting multiple times.

Relevant issues:
#555

Does this change impact existing behavior?

No, it does not change existing behaviour of any functionalities. Only change the error logs.

No breaking changes.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 01:54 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 5, 2023 01:54 — with GitHub Actions Failure
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 01:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 01:54 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 11:18 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 11:18 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 11:18 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 11:18 — with GitHub Actions Inactive
@sauraank sauraank closed this Dec 5, 2023
@sauraank sauraank reopened this Dec 5, 2023
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 12:05 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 12:05 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 5, 2023 12:05 — with GitHub Actions Failure
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 12:05 — with GitHub Actions Inactive
Copy link
Contributor

@passaro passaro left a comment

Choose a reason for hiding this comment

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

I like the log entries with the field added to the span. Just a little concerned it may be difficult to enforce that we always fill it consistently.

mountpoint-s3/src/fs.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/fuse.rs Show resolved Hide resolved
mountpoint-s3/src/fs.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
mountpoint-s3/src/inode.rs Outdated Show resolved Hide resolved
@sauraank
Copy link
Contributor Author

sauraank commented Dec 5, 2023

I like the log entries with the field added to the span. Just a little concerned it may be difficult to enforce that we always fill it consistently.

Yes, I am filling it at the first occurence of the filename in its call stack. So, any error logged before that wont have filename filled for it.

@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 23:47 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 23:47 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 5, 2023 23:47 — with GitHub Actions Failure
@sauraank sauraank temporarily deployed to PR integration tests December 5, 2023 23:47 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 00:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 00:06 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 00:06 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 6, 2023 00:06 — with GitHub Actions Failure
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 11:15 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 11:15 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 11:15 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 11:15 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 6, 2023 14:34 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 6, 2023 15:06 — with GitHub Actions Failure
@sauraank sauraank had a problem deploying to PR integration tests December 6, 2023 15:06 — with GitHub Actions Failure
Ankit Saurabh added 5 commits December 7, 2023 00:49
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:49 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:49 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:49 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:49 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:50 — with GitHub Actions Inactive
@sauraank sauraank had a problem deploying to PR integration tests December 7, 2023 00:50 — with GitHub Actions Failure
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 00:50 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank temporarily deployed to PR integration tests December 7, 2023 01:09 — with GitHub Actions Inactive
@sauraank sauraank added this pull request to the merge queue Dec 7, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 7, 2023
@sauraank sauraank added this pull request to the merge queue Dec 7, 2023
Merged via the queue into awslabs:main with commit 825bdf7 Dec 7, 2023
24 checks passed
@sauraank sauraank deleted the key_in_span branch December 7, 2023 12:06
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