feat: otelgraphql-go instrumentation #9
Conversation
gtourkas
changed the title
|
👍 Thanks for sending this. I will leave some suggestions, but I can work on them myself if you are short on time. Just let me know. What do you think about changing the high-level API // Before
otelgraphqlgo.NewOpenTelemetryTracer
// After
otelgraphql.NewTracerSpecifically:
|
|
|
||
| type OpenTelemetryTracer struct { | ||
| Tracer oteltrace.Tracer | ||
| } |
There was a problem hiding this comment.
OpenTelemetryTracer -> Tracer
I would also unexport Tracer oteltrace.Tracer field to make future changes easier.
otelgraphql-go/tracer.go
Outdated
|
|
||
| const tracerName = "github.com/uptrace/opentelemetry-go-extra/otelgraphql-go" | ||
|
|
||
| func NewOpenTelemetryTracer(opts ...Option) OpenTelemetryTracer { |
There was a problem hiding this comment.
func NewTracer(opts ...Option) *Tracer {
I suggest to return a pointer here so we can add more fields to the Tracer struct, for example, add support for metrics.
| } | ||
|
|
||
| // stolen from otellogrus | ||
| func attrAny(key string, value interface{}) attribute.KeyValue { |
There was a problem hiding this comment.
I've created #10 to clean this up
|
|
||
| if len(variables) != 0 { | ||
| for name, value := range variables { | ||
| span.SetAttributes(attrAny("graphql.variables."+name, value)) |
There was a problem hiding this comment.
I am not sure about the attribute names, but here are some ideas
- drop
trace.operation- it looks it should be part of span name - make span name more Go-like, e.g.
graphql.Requestorgraphql.+ operationName graphql.queryprobably should bedb.statementuntil the spec has a better attributegraphql.operationNamecould bedb.operation
Though I guess it is better to not argue about this right now and tweak it in subsequent PR(s)...
There was a problem hiding this comment.
graphql-go already has an opentracing Tracer implementation; this is a port of that implementation so I suggest we leave as-is for now
|
Overall this looks pretty good and the only important stuff is in this comment. We should be ready to merge this PR once it is addressed. |
No description provided.