add missing tracer for Dgraph datasource #1235
Conversation
| tracer := otel.GetTracerProvider().Tracer("gofr-dgraph") | ||
|
|
||
| db.UseTracer(tracer) |
There was a problem hiding this comment.
While this change might fix the issue, there might be a problem somewhere else.
A tracer cannot be considered as mandatory.
I mean it's a feature to have a tracer. What was the panic ?
There must be code that implies a tracer to be always present, while it could be simply absent.
There was a problem hiding this comment.
@ccoVeille I agree with your suggestion that making tracer necessary can be avoided, but since we are implementing these external datasources ourselves and as a framework we feel that observability is crucial to application, so we try to add it everywhere possible.
We just missed it in DGraph when adding for other datasources thatswhy this issue and this solution is here, but we will be extra careful when implementing datasources on our end so that it doesn't repeat.
There was a problem hiding this comment.
My problem is not functional.
I'm OK with having a tracer.
My problem is having code that panic if a tracer is not set.
Did you find where the code was panicking?
I mean was it something like foo.Bar where bar is nil, or a code that raise a panic when some variable is checked.
If it's the first option, the code needs to be updated to raise an error. Either a non blocking one, or a functional panic if the tracer is mandatory.
Also, this panic should be covered by a test that will help to either check there is no panic, or we have the expected panic
Here your change is about putting a patch on a wooden leg.
My 2 cents
There was a problem hiding this comment.
@ccoVeille addressing your comments, I have added the checks before setting the attributes to the span(which was causing panic earlier). Now even if you don't provide a tracer code will not panic and work as expected.
Pull Request Template
Description:
Checklist:
goimportandgolangci-lint.Thank you for your contribution!