Tracing #12
No reviewers
Labels
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
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Blocks
#18 Release v2.15.0}
TrueCloudLab/frostfs-api-go
Reference: TrueCloudLab/frostfs-api-go#12
Loading…
Reference in a new issue
No description provided.
Delete branch "dstepanov-yadro/frostfs-api-go:feat/OBJECT-3310"
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?
2ea9e4e19c
to6d77ad54fd
6d77ad54fd
to17019b922e
17019b922e
to19970ac8e0
19970ac8e0
to3710cd0d52
3710cd0d52
toce20d4cd50
ce20d4cd50
to2e61ee3f29
2e61ee3f29
toebf3b272ce
ebf3b272ce
to730324d177
730324d177
to04be2ef341
04be2ef341
to0da8e6c2df
0da8e6c2df
to1b90db85b1
@ -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)
This place causes the most doubts.
What doubts do you have?
I have doubts about using channels. If there are non-closed channels somewhere, then this can potentially lead to smemory leaks.
WIP: Add tracingto Tracing@ -3,6 +3,7 @@
## [Unreleased]
### Added
- Base tracing support
Link to this PR?
Also is it base or basic?
Fixed
@ -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
I would prefer updating dependencies in a separate commit.
testify was updated with otel dependency:
separate commit added.
@ -0,0 +2,4 @@
import "fmt"
// Exporter is type of tracing target
Point at the end.
Fixed
@ -0,0 +117,4 @@
ctx, span := StartSpanFromContext(ctx, method, trace.WithAttributes(
semconv.RPCSystemGRPC,
semconv.RPCMethod(method),
attribute.String("rpc.grpc.target", cc.Target())),
It it our string, or is it defined somewhere?
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 andrpc.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.
@ -0,0 +23,4 @@
c.md.Set(key, value)
}
func (c *grpcMetadataCarrier) Keys() []string {
It is unused or empty slice of
Keys
is expected? Some comment would be helpful.propagator
implements OTEL'sTextMapPropagator
interface. It requiresTextMapCarrier
instance. SogrpcMetadataCarrier
implementsTextMapCarrier
interface.grpcMetadataCarrier.Keys()
is unused bypropagator
. But I agree that it is better to implementTextMapCarrier
interface correctly. Fixed.It is possible not to use OTEL's interfaces, then
grpcMetadataCarrier.Keys()
can be deleted. But in some packagespropagator
can be used with global OTEL tracer.@ -0,0 +109,4 @@
func (cs *clientStream) RecvMsg(m interface{}) error {
err := cs.originalStream.RecvMsg(m)
if (err == nil && !cs.desc.ServerStreams) || err != nil {
Isn't it the same as
err != nil || !cs.desc.ServerStreams
?yes, it is the same. fixed.
@ -0,0 +10,4 @@
)
const (
traceIDHeader = "x-frostfs-trace-id"
Why are they private? It seems they should be set somewhere?
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.1b90db85b1
to8c68c04fb1
8c68c04fb1
to0cd07b17fa
0cd07b17fa
tof48ba1dbff
@ -0,0 +6,4 @@
type Exporter string
const (
// Stdout ...
gonna be removed?
I think we should leave this exporter, it can be useful for debugging.
oh, i meant
// *Word* ...
commentsfixed
@ -0,0 +10,4 @@
)
const (
traceIDHeader = "x-frostfs-trace-id"
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?
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.
f48ba1dbff
toa1da9033b4
a1da9033b4
to507a68cc9e
507a68cc9e
to6048ccabc9
LGTM
@ -0,0 +14,4 @@
)
// NewGRPCUnaryClientInteceptor creates new gRPC unary interceptor to save gRPC client traces.
func NewGRPCUnaryClientInteceptor() grpc.UnaryClientInterceptor {
Why did you define it as
func()
returning interceptor instead offunc GRPCUnaryClientInterceptor(ctx ..)
?It doesn't matter in this place. But often the interceptors have parameters, so they are usually created as an instance.
@ -0,0 +116,4 @@
if err != nil || !cs.desc.ServerStreams {
select {
case <-cs.done:
case cs.finished <- err:
It seems we first close
done
and thenfinished
, even though, is it guaranteed that we do not panic here?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 fromfinished
, it closes done first.@ -0,0 +14,4 @@
spanIDHeader = "x-frostfs-span-id"
flagsHeader = "x-frostfs-trace-flags"
flagsSampled = 0x01
Do we expect to have more flags? Why not using
iota
for them?Who knows?
It's a matter of opinion. I think iota is not obvious:
But fixed anyway to use iota.
@ -0,0 +16,4 @@
)
var (
tracingLock = &sync.Mutex{}
Could you add a comment about what variables it protects?
Done.
@ -0,0 +129,4 @@
case Stdout:
return stdouttrace.New()
case OTLPgRPC:
return otlptracegrpc.New(ctx, otlptracegrpc.WithEndpoint(cfg.Endpoint), otlptracegrpc.WithInsecure())
How difficult is it to allow using secure connections here? Is it enough to add fields in
Config
?Yes.
6048ccabc9
tob3d21bd65c
@ -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) {
Do we need a pointer here?
see
var Propagator propagation.TextMapPropagator = &propagator{}
There is single instance of propagator defined as pointer. So receiver is also pointer.
b3d21bd65c
to44d19a4293
44d19a4293
toe4cd5ff867
alexvanin referenced this pull request2023-04-11 07:48:57 +00:00
e4cd5ff867
tofc79fc58df
fc79fc58df
to3f5cf61b52
3f5cf61b52
toa4e361a2e6