GET/HEAD: Add tracing support #20

Merged
alexvanin merged 1 commit from dstepanov-yadro/frostfs-http-gw:feat/OBJECT-3311 into master 2023-05-04 14:01:57 +00:00
No description provided.
dstepanov-yadro force-pushed feat/OBJECT-3311 from cd87814c92 to 32ff31e941 2023-03-15 12:56:30 +00:00 Compare
alexvanin added this to the v0.28.0 milestone 2023-04-12 13:47:36 +00:00
dstepanov-yadro was assigned by alexvanin 2023-04-12 14:42:05 +00:00
dstepanov-yadro force-pushed feat/OBJECT-3311 from 32ff31e941 to 3b7eda465b 2023-04-17 10:21:58 +00:00 Compare
dstepanov-yadro requested review from storage-services-committers 2023-04-17 11:19:44 +00:00
dstepanov-yadro requested review from storage-services-developers 2023-04-17 11:19:44 +00:00
dstepanov-yadro changed title from WIP: Add tracing support to GET/HEAD: Add tracing support 2023-04-17 11:19:56 +00:00
dstepanov-yadro force-pushed feat/OBJECT-3311 from 3b7eda465b to a32ff4597f 2023-05-02 09:33:17 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3311 from a32ff4597f to da5d475374 2023-05-02 09:46:57 +00:00 Compare
dkirillov reviewed 2023-05-03 08:53:31 +00:00
app.go Outdated
@ -552,0 +555,4 @@
func (a *app) initTracing() {
instanceID := ""
servers := fetchServers(a.cfg)
Member

Why don't we use a.servers here?

Why don't we use `a.servers` here?
Author
Member

fixed

fixed
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:54:59 +00:00
app.go Outdated
@ -552,0 +567,4 @@
InstanceID: instanceID,
Version: Version,
}
updated, err := tracing.Setup(context.Background(), cfg)
Member

Why don't we get context as parameter?

Why don't we get context as parameter?
Author
Member

fixed

fixed
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-03 08:56:46 +00:00
settings.go Outdated
@ -48,2 +48,4 @@
cfgPprofAddress = "pprof.address"
// Tracing ...
cfgTracingEnabled = "tracing.enabled"
Member

We should add this new config parameters to config example and documentation

We should add this new config parameters to [config example](https://git.frostfs.info/dstepanov-yadro/frostfs-http-gw/src/branch/feat/OBJECT-3311/config) and [documentation](https://git.frostfs.info/dstepanov-yadro/frostfs-http-gw/src/branch/feat/OBJECT-3311/docs/gate-configuration.md)
Author
Member

done

done
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-03 09:04:31 +00:00
dkirillov reviewed 2023-05-03 09:10:46 +00:00
@ -89,3 +91,3 @@
}
func (r request) receiveFile(clnt *pool.Pool, objectAddress oid.Address) {
func receiveFile(ctx context.Context, req request, clnt *pool.Pool, objectAddress oid.Address) {
Member

With changing signature we can drop appCtx from request

With changing signature we can drop `appCtx` from `request`
Author
Member

Done

Done
dkirillov marked this conversation as resolved
alexvanin approved these changes 2023-05-03 10:32:13 +00:00
alexvanin left a comment
Owner

LGTM, see comments though.

LGTM, see comments though.
app.go Outdated
@ -166,6 +167,7 @@ func newApp(ctx context.Context, opt ...Option) App {
a.initAppSettings()
a.initResolver()
a.initMetrics()
a.initTracing()
Owner

Should we call tracing.Shutdown() near a.stopServices() call?

Should we call `tracing.Shutdown()` near `a.stopServices()` call?
Author
Member

Sure, fixed

Sure, fixed
alexvanin marked this conversation as resolved
utils/tracing.go Outdated
@ -0,0 +26,4 @@
func (c *httpCarrier) Set(key string, value string) {
c.r.Response.Header.Set(key, value)
}
func (c *httpCarrier) Keys() []string {
Owner

No empty line between methods?

No empty line between methods?
Author
Member

fixed

fixed
alexvanin marked this conversation as resolved
dstepanov-yadro force-pushed feat/OBJECT-3311 from da5d475374 to 476c74838a 2023-05-04 11:40:45 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3311 from 476c74838a to 0e00fdc25e 2023-05-04 11:44:42 +00:00 Compare
dkirillov reviewed 2023-05-04 12:42:27 +00:00
app.go Outdated
@ -387,0 +391,4 @@
shdnCtx, cancel := context.WithTimeout(context.Background(), tracingShutdownTimeout)
defer cancel()
if err := tracing.Shutdown(shdnCtx); err != nil {
Member

Can we move this code (that shutdown tracing) to separate function?

Can we move this code (that shutdown tracing) to separate function?
Author
Member

done

done
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-04 12:43:26 +00:00
app.go Outdated
@ -552,0 +567,4 @@
servers := a.servers
if len(servers) > 0 {
instanceID = servers[0].Address()
}
Member

Let's write:

if len(a.servers) > 0 {
	instanceID = a.servers[0].Address()
}
Let's write: ``` if len(a.servers) > 0 { instanceID = a.servers[0].Address() } ```
Author
Member

done

done
dkirillov marked this conversation as resolved
dkirillov reviewed 2023-05-04 12:46:28 +00:00
@ -241,0 +248,4 @@
tracing:
enabled: true
exporter: "otlp_grpc"
endpoint: "localhost"
Member

Is there some default port? If so, let's specify it too

Is there some default port? If so, let's specify it too
Author
Member

done

done
dkirillov marked this conversation as resolved
dstepanov-yadro force-pushed feat/OBJECT-3311 from 0e00fdc25e to a945cdd42c 2023-05-04 13:03:50 +00:00 Compare
dkirillov approved these changes 2023-05-04 13:52:33 +00:00
Member

@alexvanin Should we update CHANGELOG.md?

@alexvanin Should we update CHANGELOG.md?
Owner

@alexvanin Should we update CHANGELOG.md?

Going to update in #44

> @alexvanin Should we update CHANGELOG.md? Going to update in #44
alexvanin merged commit a945cdd42c into master 2023-05-04 14:01:57 +00:00
alexvanin referenced this pull request from a commit 2023-05-04 14:01:57 +00:00
alexvanin deleted branch feat/OBJECT-3311 2023-05-04 14:01:58 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No milestone
No project
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#20
No description provided.