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

Restore request_id to Graveler logs #4616

Merged
merged 2 commits into from
Nov 20, 2022

Conversation

arielshaqed
Copy link
Contributor

Also add request_id to some logs to actions. Fixes #4566, and helps push
#2682 towards the finish line.

Also add request_id to _some_ logs to actions.  Fixes #4566, and helps push
@arielshaqed
Copy link
Contributor Author

Tested by running locally manually. I can now see:

  • request_id on Graveler logs about branch conflicts, e.g.:
    INFO   [2022-11-20T16:52:05+02:00]pkg/graveler/graveler.go:1785 pkg/graveler.(*KVGraveler).retryBranchUpdate.func1 Retrying update branch                     branchID=main host="localhost:8000" method=POST operation_id=MergeIntoBranch path=/api/v1/repositories/bar/refs/moo/merge/main request_id=bc71651e-1999-419f-8a30-11f3a82bb939 service_name=rest_api try=2
    
  • request_id on some actions log lines, notably the "filtering" one, e.g.:
    DEBUG  [2022-11-20T16:50:57+02:00]pkg/actions/service.go:226 pkg/actions.(*StoreService).Run Filtering actions                             host="localhost:8000" method=POST operation_id=Commit path=/api/v1/repositories/bar/branches/moo/commits record="{5mt5qbv13hou1ekh8jqg pre-commit bar s3://treeverse-ariels-test-us/repos/local-bar/ moo moo {1 admin add foo  2022-11-20 16:50:57.826849025 +0200 IST m=+515.643694361 [92d1379d90d81f5da73412137b65594106b3b32fdae34b24b5ec527e7ae24cbd] map[] 0}   }" request_id=e6bb0334-60fd-44f5-ad6e-8e4fb59f4341 service_name=rest_api spec="{pre-commit moo}"
    
    

@arielshaqed arielshaqed requested a review from nopcoder November 20, 2022 15:04
@arielshaqed arielshaqed added bug Something isn't working area/KV Improvements to the KV store implementation exclude-changelog PR description should not be included in next release changelog labels Nov 20, 2022
Copy link
Contributor

@nopcoder nopcoder left a comment

Choose a reason for hiding this comment

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

LGTM, require a minor fix to the code documentation.

@@ -859,7 +859,9 @@ type KVGraveler struct {
StagingManager StagingManager
protectedBranchesManager ProtectedBranchesManager
garbageCollectionManager GarbageCollectionManager
log logging.Logger
// loggerBase enriched with context to be used for logging. It
Copy link
Contributor

Choose a reason for hiding this comment

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

need to fix to 'logger' or 'log' to match the field name

@arielshaqed
Copy link
Contributor Author

Fixed, thanks! Pulling once tests green.

@arielshaqed arielshaqed merged commit 90f7dde into master Nov 20, 2022
@arielshaqed arielshaqed deleted the bug/4566-keep-request-id-in-graveler branch November 20, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/KV Improvements to the KV store implementation bug Something isn't working exclude-changelog PR description should not be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KVGraveler loses request ID fields and other contextual fields
2 participants