Tracing #12

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-api-go:feat/OBJECT-3310 into master 2023-07-26 21:08:03 +00:00
No description provided.
dstepanov-yadro force-pushed feat/OBJECT-3310 from 2ea9e4e19c to 6d77ad54fd 2023-03-15 05:27:25 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 6d77ad54fd to 17019b922e 2023-03-15 06:17:50 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 17019b922e to 19970ac8e0 2023-03-15 06:18:08 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 19970ac8e0 to 3710cd0d52 2023-03-15 06:18:43 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 3710cd0d52 to ce20d4cd50 2023-03-15 06:59:45 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from ce20d4cd50 to 2e61ee3f29 2023-03-15 07:31:47 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 2e61ee3f29 to ebf3b272ce 2023-03-15 07:48:26 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from ebf3b272ce to 730324d177 2023-03-15 07:49:19 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 730324d177 to 04be2ef341 2023-03-15 07:49:56 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 04be2ef341 to 0da8e6c2df 2023-03-15 08:51:54 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 0da8e6c2df to 1b90db85b1 2023-03-17 12:56:24 +00:00 Compare
dstepanov-yadro reviewed 2023-03-17 13:09:43 +00:00
@ -0,0 +36,4 @@
// NewGRPCStreamClientInterceptor creates new gRPC stream interceptor to save gRPC client traces.
func NewGRPCStreamClientInterceptor() grpc.StreamClientInterceptor {
return func(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) {
ctx, span := startClientSpan(ctx, cc, method)
Author
Member

This place causes the most doubts.

This place causes the most doubts.
Owner

What doubts do you have?

What doubts do you have?
Author
Member

I have doubts about using channels. If there are non-closed channels somewhere, then this can potentially lead to smemory leaks.

I have doubts about using channels. If there are non-closed channels somewhere, then this can potentially lead to smemory leaks.
dstepanov-yadro changed title from WIP: Add tracing to Tracing 2023-03-17 13:10:24 +00:00
dstepanov-yadro requested review from storage-core-committers 2023-03-20 09:31:35 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-03-20 09:31:44 +00:00
fyrchik reviewed 2023-03-21 08:31:08 +00:00
CHANGELOG.md Outdated
@ -3,6 +3,7 @@
## [Unreleased]
### Added
- Base tracing support
Owner

Link to this PR?
Also is it base or basic?

Link to this PR? Also is it base or basic?
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
go.mod Outdated
@ -5,3 +5,3 @@
require (
git.frostfs.info/TrueCloudLab/frostfs-crypto v0.6.0
github.com/stretchr/testify v1.7.0
github.com/stretchr/testify v1.8.2
Owner

I would prefer updating dependencies in a separate commit.

I would prefer updating dependencies in a separate commit.
Author
Member

testify was updated with otel dependency:

go get go.opentelemetry.io/otel@v1.14.0
...
go: upgraded github.com/stretchr/testify v1.7.0 => v1.8.2
...

separate commit added.

testify was updated with otel dependency: ``` go get go.opentelemetry.io/otel@v1.14.0 ... go: upgraded github.com/stretchr/testify v1.7.0 => v1.8.2 ... ``` separate commit added.
fyrchik marked this conversation as resolved
@ -0,0 +2,4 @@
import "fmt"
// Exporter is type of tracing target
Owner

Point at the end.

Point at the end.
Author
Member

Fixed

Fixed
fyrchik marked this conversation as resolved
@ -0,0 +117,4 @@
ctx, span := StartSpanFromContext(ctx, method, trace.WithAttributes(
semconv.RPCSystemGRPC,
semconv.RPCMethod(method),
attribute.String("rpc.grpc.target", cc.Target())),
Owner

It it our string, or is it defined somewhere?

It it our string, or is it defined somewhere?
Author
Member

It's our. Here is Open Telemetry semantic conventions: https://pkg.go.dev/go.opentelemetry.io/otel@v1.14.0/semconv/v1.14.0
The convention has http.target key and rpc.grpc.status_code.

It will probably be correct to extract the net.host.name, net.host.port ... from Target(), but i don't think it is required.

These attributes are used to search traces in Jaeger.

It's our. Here is Open Telemetry semantic conventions: https://pkg.go.dev/go.opentelemetry.io/otel@v1.14.0/semconv/v1.14.0 The convention has ```http.target``` key and ```rpc.grpc.status_code```. It will probably be correct to extract the ```net.host.name, net.host.port ...``` from Target(), but i don't think it is required. These attributes are used to search traces in Jaeger.
fyrchik marked this conversation as resolved
@ -0,0 +23,4 @@
c.md.Set(key, value)
}
func (c *grpcMetadataCarrier) Keys() []string {
Owner

It is unused or empty slice of Keys is expected? Some comment would be helpful.

It is unused or empty slice of `Keys` is expected? Some comment would be helpful.
Author
Member

propagator implements OTEL's TextMapPropagator interface. It requires TextMapCarrier instance. So grpcMetadataCarrier implements TextMapCarrier interface.
grpcMetadataCarrier.Keys() is unused by propagator. But I agree that it is better to implement TextMapCarrier interface correctly. Fixed.

It is possible not to use OTEL's interfaces, then grpcMetadataCarrier.Keys() can be deleted. But in some packages propagator can be used with global OTEL tracer.

```propagator``` implements OTEL's ```TextMapPropagator``` interface. It requires ```TextMapCarrier``` instance. So ```grpcMetadataCarrier``` implements ```TextMapCarrier``` interface. ```grpcMetadataCarrier.Keys()``` is unused by ```propagator```. But I agree that it is better to implement ```TextMapCarrier``` interface correctly. Fixed. It is possible not to use OTEL's interfaces, then ```grpcMetadataCarrier.Keys()``` can be deleted. But in some packages ```propagator``` can be used with global OTEL tracer.
fyrchik marked this conversation as resolved
@ -0,0 +109,4 @@
func (cs *clientStream) RecvMsg(m interface{}) error {
err := cs.originalStream.RecvMsg(m)
if (err == nil && !cs.desc.ServerStreams) || err != nil {
Owner

Isn't it the same as err != nil || !cs.desc.ServerStreams?

Isn't it the same as `err != nil || !cs.desc.ServerStreams`?
Author
Member

yes, it is the same. fixed.

yes, it is the same. fixed.
fyrchik marked this conversation as resolved
@ -0,0 +10,4 @@
)
const (
traceIDHeader = "x-frostfs-trace-id"
Owner

Why are they private? It seems they should be set somewhere?

Why are they private? It seems they should be set somewhere?
Author
Member

They set by propagator. Other clients should implement TextMapCarrier interface and use it with public propagator.
For example, http-gw has TextMapCarrier implementation that reads/writes headers to http request/response.

They set by propagator. Other clients should implement ```TextMapCarrier``` interface and use it with public propagator. For example, http-gw has ```TextMapCarrier``` implementation that reads/writes headers to http request/response.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/OBJECT-3310 from 1b90db85b1 to 8c68c04fb1 2023-03-21 08:48:43 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 8c68c04fb1 to 0cd07b17fa 2023-03-21 09:17:40 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 0cd07b17fa to f48ba1dbff 2023-03-21 09:24:30 +00:00 Compare
carpawell reviewed 2023-03-21 19:02:28 +00:00
@ -0,0 +6,4 @@
type Exporter string
const (
// Stdout ...
Contributor

gonna be removed?

gonna be removed?
Author
Member

I think we should leave this exporter, it can be useful for debugging.

I think we should leave this exporter, it can be useful for debugging.
Contributor

oh, i meant // *Word* ... comments

oh, i meant `// *Word* ...` comments
Author
Member

fixed

fixed
@ -0,0 +10,4 @@
)
const (
traceIDHeader = "x-frostfs-trace-id"
Contributor

sometime ago we renamed product and had some problems with that. can that be more neutral? cc @TrueCloudLab/storage-core-committers

also, is it smth common to use "x" here?

sometime ago we renamed product and had some problems with that. can that be more neutral? cc @TrueCloudLab/storage-core-committers also, is it smth common to use "x" here?
Author
Member

I believe that this header should be specific so as not to overlap with similar user headers. Other examples are x-amzn-trace-id, uber-trace-id, x-appgw-trace-id, x-o3-trace-id.

"x-" prefix prefix was used for non-standard headers. There is no such recommendation in the standards, but many continue to add it to non-standard headers.

I believe that this header should be specific so as not to overlap with similar user headers. Other examples are x-amzn-trace-id, uber-trace-id, x-appgw-trace-id, x-o3-trace-id. "x-" prefix prefix was used for non-standard headers. There is no such recommendation in the standards, but many continue to add it to non-standard headers.
dstepanov-yadro force-pushed feat/OBJECT-3310 from f48ba1dbff to a1da9033b4 2023-03-22 13:41:29 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from a1da9033b4 to 507a68cc9e 2023-04-10 13:05:10 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 507a68cc9e to 6048ccabc9 2023-04-10 13:13:45 +00:00 Compare
dstepanov-yadro requested review from ale64bit 2023-04-10 13:20:11 +00:00
aarifullin approved these changes 2023-04-10 16:19:38 +00:00
aarifullin left a comment
Member

LGTM

LGTM
fyrchik reviewed 2023-04-11 04:56:04 +00:00
@ -0,0 +14,4 @@
)
// NewGRPCUnaryClientInteceptor creates new gRPC unary interceptor to save gRPC client traces.
func NewGRPCUnaryClientInteceptor() grpc.UnaryClientInterceptor {
Owner

Why did you define it as func() returning interceptor instead of func GRPCUnaryClientInterceptor(ctx ..)?

Why did you define it as `func()` returning interceptor instead of `func GRPCUnaryClientInterceptor(ctx ..)`?
Author
Member

It doesn't matter in this place. But often the interceptors have parameters, so they are usually created as an instance.

It doesn't matter in this place. But often the interceptors have parameters, so they are usually created as an instance.
fyrchik marked this conversation as resolved
@ -0,0 +116,4 @@
if err != nil || !cs.desc.ServerStreams {
select {
case <-cs.done:
case cs.finished <- err:
Owner

It seems we first close done and then finished, even though, is it guaranteed that we do not panic here?

It seems we first close `done` and then `finished`, even though, is it guaranteed that we do not panic here?
Author
Member

I don't see any circumstances under which panic can occur.
Both channels are unbuffered. If error occurs, only single goroutine can write an error to finished channel. After other goroutine reads error from finished, it closes done first.

I don't see any circumstances under which panic can occur. Both channels are unbuffered. If error occurs, only single goroutine can write an error to `finished` channel. After other goroutine reads error from `finished`, it closes done first.
fyrchik marked this conversation as resolved
@ -0,0 +14,4 @@
spanIDHeader = "x-frostfs-span-id"
flagsHeader = "x-frostfs-trace-flags"
flagsSampled = 0x01
Owner

Do we expect to have more flags? Why not using iota for them?

Do we expect to have more flags? Why not using `iota` for them?
Author
Member

Do we expect to have more flags?

Who knows?

Why not using iota for them?

It's a matter of opinion. I think iota is not obvious:

const (
	traceIDHeader = "x-frostfs-trace-id"
	spanIDHeader  = "x-frostfs-span-id"
	flagsHeader   = "x-frostfs-trace-flags"

	flagsSampled = 1 << iota // = 8
)


const (
	flagsSampled = 1 << iota // = 1

	traceIDHeader = "x-frostfs-trace-id"
	spanIDHeader  = "x-frostfs-span-id"
	flagsHeader   = "x-frostfs-trace-flags"
	
)

But fixed anyway to use iota.

> Do we expect to have more flags? Who knows? > Why not using iota for them? It's a matter of opinion. I think iota is not obvious: ``` const ( traceIDHeader = "x-frostfs-trace-id" spanIDHeader = "x-frostfs-span-id" flagsHeader = "x-frostfs-trace-flags" flagsSampled = 1 << iota // = 8 ) const ( flagsSampled = 1 << iota // = 1 traceIDHeader = "x-frostfs-trace-id" spanIDHeader = "x-frostfs-span-id" flagsHeader = "x-frostfs-trace-flags" ) ``` But fixed anyway to use iota.
fyrchik marked this conversation as resolved
@ -0,0 +16,4 @@
)
var (
tracingLock = &sync.Mutex{}
Owner

Could you add a comment about what variables it protects?

Could you add a comment about what variables it protects?
Author
Member

Done.

Done.
fyrchik marked this conversation as resolved
@ -0,0 +129,4 @@
case Stdout:
return stdouttrace.New()
case OTLPgRPC:
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), otlptracegrpc.WithInsecure())
Owner

How difficult is it to allow using secure connections here? Is it enough to add fields in Config?

How difficult is it to allow using secure connections here? Is it enough to add fields in `Config`?
Author
Member

Is it enough to add fields in Config?

Yes.

> Is it enough to add fields in Config? Yes.
fyrchik marked this conversation as resolved
acid-ant approved these changes 2023-04-11 07:01:49 +00:00
dstepanov-yadro force-pushed feat/OBJECT-3310 from 6048ccabc9 to b3d21bd65c 2023-04-11 07:13:15 +00:00 Compare
fyrchik reviewed 2023-04-11 07:39:39 +00:00
@ -0,0 +29,4 @@
var Propagator propagation.TextMapPropagator = &propagator{}
// Inject injects tracing info to carrier.
func (p *propagator) Inject(ctx context.Context, carrier propagation.TextMapCarrier) {
Owner

Do we need a pointer here?

Do we need a pointer here?
Author
Member

see var Propagator propagation.TextMapPropagator = &propagator{}

There is single instance of propagator defined as pointer. So receiver is also pointer.

see ```var Propagator propagation.TextMapPropagator = &propagator{}``` There is single instance of propagator defined as pointer. So receiver is also pointer.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed feat/OBJECT-3310 from b3d21bd65c to 44d19a4293 2023-04-11 07:45:38 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 44d19a4293 to e4cd5ff867 2023-04-11 07:48:02 +00:00 Compare
fyrchik added a new dependency 2023-04-11 07:48:48 +00:00
dstepanov-yadro force-pushed feat/OBJECT-3310 from e4cd5ff867 to fc79fc58df 2023-04-11 07:50:00 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from fc79fc58df to 3f5cf61b52 2023-04-11 07:54:42 +00:00 Compare
dstepanov-yadro force-pushed feat/OBJECT-3310 from 3f5cf61b52 to a4e361a2e6 2023-04-11 07:55:49 +00:00 Compare
fyrchik approved these changes 2023-04-11 08:05:43 +00:00
fyrchik merged commit a4e361a2e6 into master 2023-04-11 08:05:50 +00:00
fyrchik referenced this pull request from a commit 2023-04-11 08:05:50 +00:00
fyrchik referenced this pull request from a commit 2023-04-11 08:05:50 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#18 Release v2.15.0}
TrueCloudLab/frostfs-api-go
Reference: TrueCloudLab/frostfs-api-go#12
No description provided.