TrueCloudLab/frostfs-http-gw
14
1
Fork 17
Code Issues 7 Pull requests 2 Projects Releases 8 Activity Actions

[#44] add tracing support for upload #51

Merged
alexvanin merged 1 commit from feature/add_tracing_support into master 2023-05-30 13:12:16 +00:00
Conversation 4 Commits 1 Files changed 3 +81 -49
pogpp commented 2023-05-24 09:20:30 +00:00
Member
Copy link

Signed-off-by: Pavel Pogodaev p.pogodaev@yadro.com

Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp force-pushed feature/add_tracing_support from 8509ca45a3 to 15706725db 2023-05-24 11:46:48 +00:00 Compare
pogpp force-pushed feature/add_tracing_support from 15706725db to 78a79f4f59 2023-05-24 11:48:03 +00:00 Compare
dkirillov reviewed 2023-05-26 08:33:10 +00:00
uploader/upload.go Outdated
@ -174,3 +176,4 @@
addr.SetObject(idObj)
addr.SetContainer(*idCnr)
ctx, span := utils.StartHTTPServerSpan(u.appCtx, req, "UPLOAD Object",
dkirillov commented 2023-05-26 08:33:10 +00:00
Member
Copy link

It seemsa we have to place this span creation before line:86 ... utils.GetContainerID(...) and don't use u.appCtx after span be created

It seemsa we have to place this span creation before `line:86 ... utils.GetContainerID(...)` and don't use `u.appCtx` after span be created
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-26 08:34:23 +00:00
uploader/upload.go Outdated
@ -65,3 +67,3 @@
// Upload handles multipart upload request.
func (u *Uploader) Upload(c *fasthttp.RequestCtx) {
func (u *Uploader) Upload(req *fasthttp.RequestCtx) {
dkirillov commented 2023-05-26 08:34:23 +00:00
Member
Copy link

Do we really change this? If so, then let's change in (d *Downloader) byBucketname method

Do we really change this? If so, then let's change in `(d *Downloader) byBucketname` method
pogpp marked this conversation as resolved
dkirillov reviewed 2023-05-26 08:36:08 +00:00
downloader/download.go Outdated
@ -335,2 +335,4 @@
return
}
ctx, span := utils.StartHTTPServerSpan(ctx, c, "GET Object by bucket name",
dkirillov commented 2023-05-26 08:36:08 +00:00
Member
Copy link

The same as previous comment, we have to place this before any requests to other services, to be able provide trace info to storage node

The same as previous comment, we have to place this before any requests to other services, to be able provide trace info to storage node
pogpp marked this conversation as resolved
pogpp force-pushed feature/add_tracing_support from 78a79f4f59 to 65902754a4 2023-05-26 10:34:32 +00:00 Compare
dkirillov reviewed 2023-05-26 10:56:12 +00:00
uploader/upload.go Outdated
@ -84,1 +86,3 @@
idCnr, err := utils.GetContainerID(u.appCtx, scid, u.containerResolver)
ctx, span := utils.StartHTTPServerSpan(u.appCtx, req, "UPLOAD Object",
trace.WithAttributes(
attribute.String("scid", scid),
dkirillov commented 2023-05-26 10:56:12 +00:00
Member
Copy link

It's just cid, so let's write:

attribute.String("cid", scid),
It's just `cid`, so let's write: ```golang attribute.String("cid", scid), ```
pogpp marked this conversation as resolved
dkirillov approved these changes 2023-05-26 10:56:51 +00:00
dkirillov left a comment
Member
Copy link

LGMT, see minor comment

LGMT, see minor comment
pogpp force-pushed feature/add_tracing_support from 65902754a4 to 8d5c2997ac 2023-05-26 11:35:25 +00:00 Compare
dkirillov approved these changes 2023-05-26 12:21:40 +00:00
alexvanin requested changes 2023-05-26 13:01:10 +00:00
alexvanin left a comment
Owner
Copy link

Please look at the byAttribute code. We call d.search() which produces object.search request to FrostFS, we would like to trace them as well.

Please look at the `byAttribute` code. We call `d.search()` which produces object.search request to FrostFS, we would like to trace them as well.
alexvanin commented 2023-05-26 13:11:31 +00:00
Owner
Copy link

It seems we miss tracing in zip download, see d.zipObject too.

It seems we miss tracing in zip download, see `d.zipObject` too.
alexvanin commented 2023-05-26 13:34:36 +00:00
Owner
Copy link

And we should not use utils.StartHTTPServerSpan more than once in the execution stack. Let's use it once in the handler function (e.g. byAttribute) and use tracing.StartSpanFromContext down in stack (e.g. in headObject).

And we **should not** use `utils.StartHTTPServerSpan` more than once in the execution stack. Let's use it once in the handler function (e.g. `byAttribute`) and use [tracing.StartSpanFromContext](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/branch/master/pkg/local_object_storage/shard/lock.go#L22) down in stack (e.g. in `headObject`).
pogpp added 1 commit 2023-05-29 11:16:57 +00:00 
Signed-off-by: Pavel Pogodaev <p.pogodaev@yadro.com>
pogpp requested review from alexvanin 2023-05-29 11:17:58 +00:00
pogpp requested review from dkirillov 2023-05-29 13:53:28 +00:00
dkirillov reviewed 2023-05-30 09:06:47 +00:00
downloader/download.go Outdated
@ -405,3 +416,3 @@
}
func (d *Downloader) search(c *fasthttp.RequestCtx, cid *cid.ID, key, val string, op object.SearchMatchType) (pool.ResObjectSearch, error) {
func (d *Downloader) search(c *fasthttp.RequestCtx, ctx context.Context, cid *cid.ID, key, val string, op object.SearchMatchType) (pool.ResObjectSearch, error) {
dkirillov commented 2023-05-30 09:01:17 +00:00
Member
Copy link

Drop c *fasthttp.RequestCtx and store bearer token in ctx.
So you can use the following code in DownloadZipped function (probably we can rename StoreBearerTokenAppCtx to StoreBearerTokenToCtx)

if ctx, err = tokens.StoreBearerTokenAppCtx(c, ctx); err != nil {
	// ...
}

// ...

resSearch, err := d.search(ctx, containerID, object.AttributeFilePath, prefix, object.MatchCommonPrefix)

// ...

This also require to add

if ctx, err = tokens.StoreBearerTokenAppCtx(c, ctx); err != nil {
	// ...
}

to byAttribute before invoke search

Also, having tokens.StoreBearerTokenAppCtx in byAttribute can simplify getting bearer token in receiveFile and headObject.
We should use just bearerToken(ctx) there

Drop `c *fasthttp.RequestCtx` and store bearer token in `ctx`. So you can use the following code in `DownloadZipped` function (probably we can rename `StoreBearerTokenAppCtx` to `StoreBearerTokenToCtx`) ```golang if ctx, err = tokens.StoreBearerTokenAppCtx(c, ctx); err != nil { // ... } // ... resSearch, err := d.search(ctx, containerID, object.AttributeFilePath, prefix, object.MatchCommonPrefix) // ... ``` This also require to add ```golang if ctx, err = tokens.StoreBearerTokenAppCtx(c, ctx); err != nil { // ... } ``` to `byAttribute` before invoke [search](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/a0e6eba6f7ee730165dacc5968cd7d073cd58642/downloader/download.go#L387) Also, having `tokens.StoreBearerTokenAppCtx` in `byAttribute` can simplify getting bearer token in [receiveFile](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/a0e6eba6f7ee730165dacc5968cd7d073cd58642/downloader/download.go#L100-L108) and [headObject](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/a0e6eba6f7ee730165dacc5968cd7d073cd58642/downloader/head.go#L43-L49). We should use just `bearerToken(ctx)` there
dkirillov commented 2023-05-30 11:11:37 +00:00
Member
Copy link

Now, we can drop this and this

Now, we can drop [this](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/1776db289c54088ef4ba6bdec26a7c5f2294e790/downloader/download.go#L100-L104) and [this](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/1776db289c54088ef4ba6bdec26a7c5f2294e790/downloader/head.go#L43-L47)
pogpp marked this conversation as resolved
downloader/head.go
@ -34,3 +33,1 @@
attribute.String("cid", objectAddress.Container().EncodeToString()),
attribute.String("oid", objectAddress.Object().EncodeToString()),
))
ctx, span := tracing.StartSpanFromContext(ctx, "HEAD Object", trace.WithAttributes(
dkirillov commented 2023-05-30 08:46:19 +00:00
Member
Copy link

Do we really need this new span if we have already started one here and the second will be started in SDK right in clnt.HeadObject(ctx, prm)

Do we really need this new span if we have already started one [here](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/a0e6eba6f7ee730165dacc5968cd7d073cd58642/downloader/download.go#L369) and the second will be started in SDK right in `clnt.HeadObject(ctx, prm)`
alexvanin commented 2023-05-30 09:48:40 +00:00
Owner
Copy link

In fact, client does not start a new span. Storage node will start a new span by receiving new request, providing cid and oid as attributes.

I suggest to leave this code until we refactor span initialization as a middleware as we discussed before.

In fact, client does not start a new span. Storage node will start a new span by receiving new request, providing `cid` and `oid` as attributes. I suggest to leave this code until we refactor span initialization as a middleware as we discussed before.
dkirillov commented 2023-05-30 11:01:48 +00:00
Member
Copy link

Shouldn't we add the similar span to receiveFile then, to be more consistent ?

Shouldn't we add the similar span to [receiveFile](https://git.frostfs.info/TrueCloudLab/frostfs-http-gw/src/commit/1776db289c54088ef4ba6bdec26a7c5f2294e790/downloader/download.go#L93) then, to be more consistent ?
pogpp commented 2023-05-30 11:09:50 +00:00
Author
Member
Copy link
func receiveFile(ctx context.Context, req request, clnt *pool.Pool, objectAddress oid.Address) {
	var (
		err      error
		dis      = "inline"
		start    = time.Now()
		filename string
	)

	ctx, span := utils.StartHTTPServerSpan(ctx, req.RequestCtx, "RECEIVE Object",
		trace.WithAttributes(
			attribute.String("objectAddress", objectAddress.String()),
		))
	defer func() {
		utils.SetHTTPTraceInfo(ctx, span, c)
		span.End()
	}()

Like that?

``` func receiveFile(ctx context.Context, req request, clnt *pool.Pool, objectAddress oid.Address) { var ( err error dis = "inline" start = time.Now() filename string ) ctx, span := utils.StartHTTPServerSpan(ctx, req.RequestCtx, "RECEIVE Object", trace.WithAttributes( attribute.String("objectAddress", objectAddress.String()), )) defer func() { utils.SetHTTPTraceInfo(ctx, span, c) span.End() }() ``` Like that?
dkirillov commented 2023-05-30 11:16:41 +00:00
Member
Copy link

Yes, or just exact copy from headObject:

ctx, span := tracing.StartSpanFromContext(ctx,"GET Object",trace.WithAttributes(
	attribute.String("cid", objectAddress.Container().EncodeToString()),
	attribute.String("oid", objectAddress.Object().EncodeToString()),
))
defer func() {
	utils.SetHTTPTraceInfo(ctx, span, req.RequestCtx)
	span.End()
}()
Yes, or just exact copy from `headObject`: ```golang ctx, span := tracing.StartSpanFromContext(ctx,"GET Object",trace.WithAttributes( attribute.String("cid", objectAddress.Container().EncodeToString()), attribute.String("oid", objectAddress.Object().EncodeToString()), )) defer func() { utils.SetHTTPTraceInfo(ctx, span, req.RequestCtx) span.End() }() ```
dkirillov marked this conversation as resolved
alexvanin approved these changes 2023-05-30 09:49:03 +00:00
alexvanin left a comment
Owner
Copy link

Overall LGTM, see d.search comment.

Overall LGTM, see `d.search` comment.
pogpp force-pushed feature/add_tracing_support from a0e6eba6f7 to 68c1b384e2 2023-05-30 10:56:44 +00:00 Compare
dkirillov commented 2023-05-30 11:02:55 +00:00
Member
Copy link

@pogpp Please rebase to eliminate merge commit

@pogpp Please rebase to eliminate merge commit
pogpp force-pushed feature/add_tracing_support from 68c1b384e2 to 6aeaa07789 2023-05-30 11:15:17 +00:00 Compare
pogpp force-pushed feature/add_tracing_support from 6aeaa07789 to d51c22a74f 2023-05-30 11:20:19 +00:00 Compare
dkirillov reviewed 2023-05-30 11:21:01 +00:00
downloader/download.go Outdated
@ -104,1 +101,3 @@
}
ctx, span := tracing.StartSpanFromContext(ctx, "GET Object", trace.WithAttributes(
attribute.String("cid", objectAddress.Container().EncodeToString()),
dkirillov commented 2023-05-30 11:21:01 +00:00
Member
Copy link

We should use utils.StartSpanFromContext at least. Also it would be nice to have the same attributes as in headObject

We should use `utils.StartSpanFromContext` at least. Also it would be nice to have the same attributes as in `headObject`
pogpp commented 2023-05-30 11:31:22 +00:00
Author
Member
Copy link

But we don't have utils.StartSpanFromContext we using straight tracing pkg func StartSpanFromContext

But we don't have `utils.StartSpanFromContext` we using straight tracing pkg func StartSpanFromContext
👀 1
dkirillov marked this conversation as resolved
pogpp force-pushed feature/add_tracing_support from d51c22a74f to 8a22991326 2023-05-30 13:11:15 +00:00 Compare
alexvanin commented 2023-05-30 13:12:02 +00:00
Owner
Copy link

Let's merge it here and refactor in separate PR

Let's merge it here and refactor in separate PR
alexvanin merged commit 8a22991326 into master 2023-05-30 13:12:16 +00:00
alexvanin deleted branch feature/add_tracing_support 2023-05-30 13:12:17 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
dkirillov
alexvanin
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
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
good first issue
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
3 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-http-gw#51
No description provided.
Delete branch "feature/add_tracing_support"

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?