TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 108 Pull requests 9 Projects Releases 2 Activity Actions

morph: Add tracing for morph queries #1613

Merged
fyrchik merged 1 commit from achuprov/frostfs-node:feat/morph_trace into master 2025-02-06 08:28:05 +00:00
Conversation 2 Commits 1 Files changed 120 +623 -562
achuprov commented 2025-01-23 15:40:06 +00:00
Member
Copy link

The trace is added only when a request is made to neo-go. If the data is in the cache, no entry will be added.
For example, the object put operation with disabled morph cache:

image

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

The trace is added only when a request is made to neo-go. If the data is in the cache, no entry will be added. For example, the `object put` operation with **disabled** `morph cache`: ![image](/attachments/8587b35d-8965-4121-b437-0f02a7663053) Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
image.png
139 KiB
requested reviews from storage-core-committers, storage-core-developers 2025-01-23 15:40:07 +00:00
achuprov force-pushed feat/morph_trace from 56a463e558 to 9e1925f70c 2025-01-23 15:42:05 +00:00 Compare
achuprov changed title from morph: Add tracing for morph queries in neo-go to morph: Add tracing for morph queries 2025-01-23 15:42:46 +00:00
dstepanov-yadro requested changes 2025-01-24 07:45:58 +00:00
Dismissed
cmd/frostfs-node/cache.go Outdated
@ -166,3 +167,3 @@
}
func newCachedContainerStorage(v container.Source, ttl time.Duration, containerCacheSize uint32) ttlContainerStorage {
func newCachedContainerStorage(ctx context.Context, v container.Source, ttl time.Duration, containerCacheSize uint32) ttlContainerStorage {
dstepanov-yadro commented 2025-01-24 07:44:35 +00:00
Member
Copy link

In this case, it is necessary to use not the context from the newCachedContainerStorage method, but from the call:

func newCachedContainerStorage(v container.Source, ttl time.Duration, containerCacheSize uint32) ttlContainerStorage {
	lruCnrCache := newNetworkTTLCache(int(containerCacheSize), ttl, func(ctx context.Context, id cid.ID) (*container.Container, error) {
		return v.Get(ctx, id)
	}, metrics.NewCacheMetrics("container"))
...
In this case, it is necessary to use not the context from the `newCachedContainerStorage` method, but from the call: ``` func newCachedContainerStorage(v container.Source, ttl time.Duration, containerCacheSize uint32) ttlContainerStorage { lruCnrCache := newNetworkTTLCache(int(containerCacheSize), ttl, func(ctx context.Context, id cid.ID) (*container.Container, error) { return v.Get(ctx, id) }, metrics.NewCacheMetrics("container")) ... ```
achuprov commented 2025-01-24 10:19:52 +00:00
Author
Member
Copy link

This tracing is created during the startup and initialization of the node. Do we want to explore the node startup process using tracing in the future?

This tracing is created during the startup and initialization of the node. Do we want to explore the node startup process using tracing in the future?
dstepanov-yadro commented 2025-01-24 10:48:39 +00:00
Member
Copy link

func newCachedContainerStorage(ctx context.Context - this startup context will be used for every request. I think it's wrong.

`func newCachedContainerStorage(ctx context.Context` - this startup context will be used for every request. I think it's wrong.
achuprov commented 2025-01-24 13:55:59 +00:00
Author
Member
Copy link

fixed

fixed
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro requested changes 2025-01-24 08:11:05 +00:00
Dismissed
cmd/frostfs-node/frostfsid.go Outdated
@ -43,3 +46,3 @@
}
func (m *morphFrostfsIDCache) GetSubject(addr util.Uint160) (*client.Subject, error) {
func (m *morphFrostfsIDCache) GetSubject(ctx context.Context, addr util.Uint160) (*client.Subject, error) {
dstepanov-yadro commented 2025-01-24 07:48:53 +00:00
Member
Copy link

This assertion is violated: If the data is in the cache, no entry will be added.

This assertion is violated: `If the data is in the cache, no entry will be added.`
achuprov commented 2025-01-24 10:15:43 +00:00
Author
Member
Copy link

I forgot to remove these traces. They remained after I tested the cache functionality.

I forgot to remove these traces. They remained after I tested the cache functionality.
dstepanov-yadro marked this conversation as resolved
cmd/frostfs-node/frostfsid.go Outdated
@ -56,3 +63,3 @@
}
subj, err := m.subjProvider.GetSubject(addr)
ctx, span1 := tracing.StartSpanFromContext(ctx, "subjProvider.GetSubject",
dstepanov-yadro commented 2025-01-24 07:47:39 +00:00
Member
Copy link

I would expect to see this span inside the subjProvider.

I would expect to see this span inside the subjProvider.
achuprov commented 2025-01-24 10:16:18 +00:00
Author
Member
Copy link

Already 35b711e9f9/pkg/morph/client/frostfsid/subject.go (L21)

Already https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/35b711e9f918bf8563ab63b99aee228c7608a785/pkg/morph/client/frostfsid/subject.go#L21
dstepanov-yadro marked this conversation as resolved
achuprov force-pushed feat/morph_trace from 9e1925f70c to 35b711e9f9 2025-01-24 10:11:40 +00:00 Compare
achuprov force-pushed feat/morph_trace from 35b711e9f9 to 84df5bea06 2025-01-24 13:41:06 +00:00 Compare
achuprov force-pushed feat/morph_trace from 84df5bea06 to 45d42d5a83 2025-01-24 13:49:31 +00:00 Compare
achuprov force-pushed feat/morph_trace from 45d42d5a83 to 493667a1b7 2025-01-24 13:54:10 +00:00 Compare
dstepanov-yadro reviewed 2025-01-24 14:18:52 +00:00
cmd/frostfs-node/object.go Outdated
@ -124,3 +124,3 @@
func (fn *innerRingFetcherWithNotary) InnerRingKeys() ([][]byte, error) {
func (fn *innerRingFetcherWithNotary) InnerRingKeys(_ context.Context) ([][]byte, error) {
keys, err := fn.sidechain.NeoFSAlphabetList()
dstepanov-yadro commented 2025-01-24 14:18:52 +00:00
Member
Copy link

Does NeoFSAlphabetList make RPC call? If yes, then please add tracing span there. InnerRing keys could be request for every object request.

Does `NeoFSAlphabetList` make RPC call? If yes, then please add tracing span there. InnerRing keys could be request for every object request.
achuprov commented 2025-01-24 14:57:30 +00:00
Author
Member
Copy link

Added

Added
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro commented 2025-01-24 14:19:06 +00:00
Member
Copy link

Great job!

Great job!
achuprov force-pushed feat/morph_trace from 493667a1b7 to ca50693ac6 2025-01-24 14:48:18 +00:00 Compare
dstepanov-yadro approved these changes 2025-01-24 15:02:58 +00:00
Dismissed
achuprov force-pushed feat/morph_trace from ca50693ac6 to 716494c411 2025-01-28 11:38:10 +00:00 Compare
achuprov dismissed dstepanov-yadro's review 2025-01-28 11:38:11 +00:00
Reason:

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

achuprov force-pushed feat/morph_trace from 716494c411 to 1f44f61238 2025-01-28 11:41:27 +00:00 Compare
fyrchik requested changes 2025-01-29 10:47:34 +00:00
Dismissed
pkg/morph/client/container/get.go Outdated
@ -45,2 +48,3 @@
// storage.ErrNotFound error is returned.
func (c *Client) Get(cid []byte) (*containercore.Container, error) {
func (c *Client) Get(ctx context.Context, cid []byte) (*containercore.Container, error) {
_, span := tracing.StartSpanFromContext(ctx, "GetContainer",
fyrchik commented 2025-01-29 10:46:14 +00:00
Owner
Copy link

Please, remove this span in favor of generic TestInvoke one.

Please, remove this span in favor of generic `TestInvoke` one.
pkg/morph/client/static.go Outdated
@ -208,1 +210,3 @@
func (s StaticClient) TestInvoke(prm TestInvokePrm) ([]stackitem.Item, error) {
func (s StaticClient) TestInvoke(ctx context.Context, prm TestInvokePrm) ([]stackitem.Item, error) {
_, span := tracing.StartSpanFromContext(ctx, prm.method,
trace.WithAttributes())
fyrchik commented 2025-01-29 10:47:32 +00:00
Owner
Copy link

I think span name should be generic and attributes should contain method.
Different contracts may have methods with the same name.

I think span name should be generic and attributes should contain method. Different contracts may have methods with the same name.
achuprov commented 2025-01-29 12:39:41 +00:00
Author
Member
Copy link

I agree that this is conceptually correct. However, the Jaeger UI displays the operation name by default, while attributes are hidden. This makes the trace less visually clear. image

I agree that this is conceptually correct. However, the Jaeger UI displays the operation name by default, while attributes are hidden. This makes the trace less visually clear. ![image](/attachments/403f8c4c-34ce-4b5c-9e63-31e916324b57)
image.png
136 KiB
achuprov commented 2025-01-29 12:44:30 +00:00
Author
Member
Copy link

What about the name format Morph.TestInvoke.ContractName?

What about the name format `Morph.TestInvoke.ContractName`?
fyrchik commented 2025-01-29 12:54:22 +00:00
Owner
Copy link

We should not make our decisions based on the default behaviour of some UI.
Having multiple get strings won't make things clear.
Even though we do not have a contract name here, the event is TestInvoke, not get or getSubject.
Ideally, sth like /morph/TestInvoke/netmap.snapshot would be nice, but we can postpone this.

@dstepanov-yadro other thoughts?

We should not make our decisions based on the default behaviour of some UI. Having multiple `get` strings won't make things clear. Even though we do not have a contract _name_ here, the event is `TestInvoke`, not `get` or `getSubject`. Ideally, sth like `/morph/TestInvoke/netmap.snapshot` would be nice, but we can postpone this. @dstepanov-yadro other thoughts?
fyrchik commented 2025-01-29 12:55:14 +00:00
Owner
Copy link

What about the name format Morph.TestInvoke.ContractName?

Looks ok to me, but we need to provide contract name somehow (another refactoring).

> What about the name format `Morph.TestInvoke.ContractName`? Looks ok to me, but we need to provide contract name somehow (another refactoring).
dstepanov-yadro commented 2025-01-31 06:23:32 +00:00
Member
Copy link

I think that if TestInvoke's parent span will be defined and it will be clear, then it is ok to have only TestInvoke span.

I think that if `TestInvoke`'s parent span will be defined and it will be clear, then it is ok to have only TestInvoke span.
achuprov force-pushed feat/morph_trace from 1f44f61238 to fbd08932db 2025-01-29 14:29:50 +00:00 Compare
achuprov force-pushed feat/morph_trace from fbd08932db to c64ffe2fb3 2025-01-29 14:30:05 +00:00 Compare
achuprov force-pushed feat/morph_trace from c64ffe2fb3 to 2ef8c21e90 2025-02-03 08:16:59 +00:00 Compare
acid-ant approved these changes 2025-02-03 09:15:50 +00:00
Dismissed
fyrchik requested changes 2025-02-05 11:25:55 +00:00
Dismissed
cmd/frostfs-node/frostfsid.go Outdated
@ -64,12 +64,11 @@ func (m *morphFrostfsIDCache) GetSubject(addr util.Uint160) (*client.Subject, er
}
return nil, err
}
fyrchik commented 2025-02-05 11:24:30 +00:00
Owner
Copy link

unrelated to the commit

unrelated to the commit
cmd/frostfs-node/frostfsid.go Outdated
@ -54,8 +55,7 @@ func (m *morphFrostfsIDCache) GetSubject(addr util.Uint160) (*client.Subject, er
hit = true
return result.subject, result.err
}
fyrchik commented 2025-02-05 11:24:22 +00:00
Owner
Copy link

unnessesary change

unnessesary change
fyrchik commented 2025-02-05 11:26:09 +00:00
Owner
Copy link

Please, fix 2 issues and we will merge

Please, fix 2 issues and we will merge
achuprov force-pushed feat/morph_trace from 2ef8c21e90 to eae9debc14 2025-02-05 13:38:08 +00:00 Compare
achuprov dismissed acid-ant's review 2025-02-05 13:38:08 +00:00
Reason:

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

achuprov force-pushed feat/morph_trace from eae9debc14 to 9b113c3156 2025-02-05 13:38:30 +00:00 Compare
fyrchik approved these changes 2025-02-06 08:28:01 +00:00
fyrchik merged commit 9b113c3156 into master 2025-02-06 08:28:05 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
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
4 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#1613
No description provided.
Delete branch "achuprov/frostfs-node:feat/morph_trace"

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?