TrueCloudLab/frostfs-node
21
1
Fork 27
Code Issues 107 Pull requests 6 Projects Releases 2 Activity Actions

Logger with context #1437

Merged
fyrchik merged 8 commits from dstepanov-yadro/frostfs-node:feat/log_with_ctx into master 2024-11-20 15:43:53 +00:00
Conversation 1 Commits 8 Files changed 302 +2074 -1924
dstepanov-yadro commented 2024-10-18 10:18:31 +00:00
Member
Copy link

Now log has traceID:

debug	session/executor.go:36	serving request...	{"component": "SessionService", "request": "Create", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"}
debug	log/log.go:13	local object storage operation	{"shard_id": "8VjboWGs3gVeJc8nC1TNkK", "address": "9v4dwrRSGifKt7HzpT9cN49EYLEPsAoFtsiNpa3cGbeL/HPvge2FoM6givX82PLAFxhnErNYicEENbfPfNax7gmTv", "type": "write-cache", "op": "fstree PUT", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"}
debug	log/log.go:13	local object storage operation	{"shard_id": "8VjboWGs3gVeJc8nC1TNkK", "address": "9v4dwrRSGifKt7HzpT9cN49EYLEPsAoFtsiNpa3cGbeL/HPvge2FoM6givX82PLAFxhnErNYicEENbfPfNax7gmTv", "op": "metabase PUT", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"}
Now log has traceID: ``` debug session/executor.go:36 serving request... {"component": "SessionService", "request": "Create", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"} debug log/log.go:13 local object storage operation {"shard_id": "8VjboWGs3gVeJc8nC1TNkK", "address": "9v4dwrRSGifKt7HzpT9cN49EYLEPsAoFtsiNpa3cGbeL/HPvge2FoM6givX82PLAFxhnErNYicEENbfPfNax7gmTv", "type": "write-cache", "op": "fstree PUT", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"} debug log/log.go:13 local object storage operation {"shard_id": "8VjboWGs3gVeJc8nC1TNkK", "address": "9v4dwrRSGifKt7HzpT9cN49EYLEPsAoFtsiNpa3cGbeL/HPvge2FoM6givX82PLAFxhnErNYicEENbfPfNax7gmTv", "op": "metabase PUT", "trace_id": "87a49bd0186e1d263baf077ed7b9f6de"} ```
dstepanov-yadro force-pushed feat/log_with_ctx from 8bf0a66e14 to 692749dba1 2024-10-21 07:58:58 +00:00 Compare
a-savchuk reviewed 2024-10-21 08:11:42 +00:00
pkg/util/logger/log.go Outdated
@ -0,0 +16,4 @@
}
func (l *Logger) Warn(ctx context.Context, msg string, fields ...zap.Field) {
l.z.Info(msg, append(fields, zap.String("trace_id", tracing.GetTraceID(ctx)))...)
a-savchuk commented 2024-10-21 08:11:42 +00:00
Member
Copy link

Did you mean l.z.Warn(...)?

Did you mean `l.z.Warn(...)`?
dstepanov-yadro commented 2024-10-21 13:33:00 +00:00
Author
Member
Copy link

sure, fixed, thanks

sure, fixed, thanks
a-savchuk marked this conversation as resolved
dstepanov-yadro added 5 commits 2024-10-21 13:27:58 +00:00 
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
[#1437] node: Fix contextcheck linter
Some checks failed
DCO action / DCO (pull_request) Failing after 1m29s
Details
Tests and linters / Run gofumpt (pull_request) Successful in 1m25s
Details
Pre-commit hooks / Pre-commit (pull_request) Successful in 2m23s
Details
Vulncheck / Vulncheck (pull_request) Successful in 2m17s
Details
Build / Build Components (pull_request) Successful in 2m31s
Details
Tests and linters / gopls check (pull_request) Successful in 2m50s
Details
Tests and linters / Staticcheck (pull_request) Successful in 2m59s
Details
Tests and linters / Lint (pull_request) Successful in 3m37s
Details
Tests and linters / Tests (pull_request) Successful in 4m22s
Details
Tests and linters / Tests with -race (pull_request) Successful in 5m57s
Details
16c66065e9 
Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro force-pushed feat/log_with_ctx from 16c66065e9 to 242033b436 2024-10-21 13:34:23 +00:00 Compare
dstepanov-yadro force-pushed feat/log_with_ctx from 242033b436 to f729d1a8c8 2024-11-07 11:00:43 +00:00 Compare
dstepanov-yadro changed title from WIP: Logger with context to Logger with context 2024-11-07 11:07:00 +00:00
fyrchik reviewed 2024-11-07 11:15:14 +00:00
fyrchik left a comment
Owner
Copy link

Have you seen any other implementations like
https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go
or suggestions in https://github.com/uber-go/zap/issues/654

To me, the approach with a different method name or log.Ctx(ctx).Info(...) looks clearer and is less invasive.
Having to provide context each time I need to log something would certainly introduce friction.

Have you seen any other implementations like https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go or suggestions in https://github.com/uber-go/zap/issues/654 To me, the approach with a different method name or `log.Ctx(ctx).Info(...)` looks clearer and is less invasive. _Having to_ provide context each time I need to log something would certainly introduce friction.
dstepanov-yadro force-pushed feat/log_with_ctx from f729d1a8c8 to 8b637b136b 2024-11-07 11:42:15 +00:00 Compare
dstepanov-yadro commented 2024-11-07 11:51:37 +00:00
Author
Member
Copy link

Have you seen any other implementations like
https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go
or suggestions in https://github.com/uber-go/zap/issues/654

To me, the approach with a different method name or log.Ctx(ctx).Info(...) looks clearer and is less invasive.
Having to provide context each time I need to log something would certainly introduce friction.

I think that passing the context is not as difficult as it seems at first glance. At the same time, I proceed from the opinion that people are lazy, and if something can be omitted, they do not do it.
In particular, we already have a method for logging traceId, but it was used only where it was originally done, and not everywhere where it is necessary.

Also you can use context.Background() if there is no context. But contextcheck linter will help you, if you have forgotten to use context from caller:)

> Have you seen any other implementations like > https://github.com/uptrace/opentelemetry-go-extra/blob/main/otelzap/otelzap.go > or suggestions in https://github.com/uber-go/zap/issues/654 > > To me, the approach with a different method name or `log.Ctx(ctx).Info(...)` looks clearer and is less invasive. > _Having to_ provide context each time I need to log something would certainly introduce friction. I think that passing the context is not as difficult as it seems at first glance. At the same time, I proceed from the opinion that people are lazy, and if something can be omitted, they do not do it. In particular, we already have a method for logging traceId, but it was used only where it was originally done, and not everywhere where it is necessary. Also you can use `context.Background()` if there is no context. But `contextcheck` linter will help you, if you have forgotten to use context from caller:)
dstepanov-yadro force-pushed feat/log_with_ctx from 8b637b136b to 4b5c2113c5 2024-11-07 11:55:28 +00:00 Compare
dstepanov-yadro force-pushed feat/log_with_ctx from 4b5c2113c5 to ca988524ad 2024-11-07 12:03:16 +00:00 Compare
aarifullin approved these changes 2024-11-07 15:33:11 +00:00
Dismissed
dstepanov-yadro force-pushed feat/log_with_ctx from ca988524ad to 630d996fe8 2024-11-08 14:02:48 +00:00 Compare
dstepanov-yadro dismissed aarifullin's review 2024-11-08 14:02:48 +00:00
Reason:

New commits pushed, approval review dismissed automatically according to repository settings

dstepanov-yadro force-pushed feat/log_with_ctx from 630d996fe8 to a4e10975fc 2024-11-11 09:24:21 +00:00 Compare
dstepanov-yadro force-pushed feat/log_with_ctx from a4e10975fc to ff1e7b531f 2024-11-11 11:56:54 +00:00 Compare
aarifullin reviewed 2024-11-12 12:16:47 +00:00
pkg/innerring/processors/netmap/handlers_test.go Outdated
@ -365,3 +366,3 @@
}
func (c *testNetmapClient) MorphNotaryInvoke(contract util.Uint160, fee fixedn.Fixed8, nonce uint32, vub *uint32, method string, args ...any) error {
func (c *testNetmapClient) MorphNotaryInvoke(ctx context.Context, contract util.Uint160, fee fixedn.Fixed8, nonce uint32, vub *uint32, method string, args ...any) error {
aarifullin commented 2024-11-12 12:16:47 +00:00
Member
Copy link

Interesting, linter has ignored this unused ctx?

Interesting, linter has ignored this unused `ctx`?
dstepanov-yadro force-pushed feat/log_with_ctx from ff1e7b531f to 6e283295a2 2024-11-13 07:22:08 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2024-11-13 07:25:25 +00:00
dstepanov-yadro requested review from storage-core-developers 2024-11-13 07:25:27 +00:00
dstepanov-yadro force-pushed feat/log_with_ctx from 6e283295a2 to 612b34d570 2024-11-13 07:36:20 +00:00 Compare
aarifullin approved these changes 2024-11-13 07:46:32 +00:00
aarifullin left a comment
Member
Copy link

Great

Great
a-savchuk approved these changes 2024-11-13 08:11:21 +00:00
achuprov approved these changes 2024-11-13 08:12:26 +00:00
fyrchik merged commit 612b34d570 into master 2024-11-13 08:12:47 +00:00
fyrchik referenced this pull request from a commit 2024-11-13 08:12:49 +00:00
[#1437] node: Use ctx for logging
fyrchik referenced this pull request from a commit 2024-11-13 08:12:49 +00:00
[#1437] ir: Fix contextcheck linter
fyrchik referenced this pull request from a commit 2024-11-13 08:12:49 +00:00
[#1437] ir: Fix contextcheck linters
fyrchik referenced this pull request from a commit 2024-11-13 08:12:49 +00:00
[#1437] node: Fix contextcheck linter
acid-ant approved these changes 2024-11-13 08:14:02 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
aarifullin
a-savchuk
achuprov
acid-ant
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#1437
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:feat/log_with_ctx"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?