Observability #133
Observability #133
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #133 +/- ##
==========================================
+ Coverage 73.26% 73.76% +0.49%
==========================================
Files 17 17
Lines 1343 1395 +52
==========================================
+ Hits 984 1029 +45
- Misses 266 273 +7
Partials 93 93 ☔ View full report in Codecov by Sentry. |
impl/pkg/telemetry/telemetry.go
Outdated
| ) | ||
|
|
||
| func init() { | ||
| buildInfo, ok := debug.ReadBuildInfo() |
There was a problem hiding this comment.
interesting...how does this work?
we do/should have a version set in our config
There was a problem hiding this comment.
Read about it here, right now it's just returning (devel) for the current module's version:
I thought that would become a useful name when we add tags, but I just tried adding a tag and it didn't help, so I might try to push that forward a little before merging this.
There was a problem hiding this comment.
Okay so i thought i had successfully used this in the past but i guess it will never work. Looking at where I thought I had successfully used it, I appear to have some compile-time variable setting to inject a version externally. Will add the same here.
There was a problem hiding this comment.
looks slightly better now. GitHub doesn't give us a lot of great options, and because the Dockerfile copies the impl/ not the git root, we can't use git describe or anything like that in the container. For tagged releases, the string telemetry in that screenshot should be the tag name.
metrics generated from traces are probably sufficient for our current needs, so may address metric goals as well
impl/pkg/telemetry/telemetry.go
Outdated
| const scopeName = "github.com/TBD54566975/did-dht-method" | ||
|
|
||
| var ( | ||
| Tracer trace.Tracer |
There was a problem hiding this comment.
hmm, don't love the idea of having a global tracer
can we put this on a struct which SetupTelemetry returns?
There was a problem hiding this comment.
Totally could. Is there a problem with having a global tracer? An alternative to having a tracer that gets passed around everywhere is to initialize a tracer in each package that we can use to instrument that package, which is what the gin middleware does.
There was a problem hiding this comment.
Global state scares me generally because of assumptions it caries - may/not be initialized, different callers using it for different purposes in an unsafe manner. Of course this is negligible given how small the codebase is now, though I view it as a best practice to avoid globally mutable state
There was a problem hiding this comment.
Reasonable. I'll figure out how I want to setup non-global tracer(s)
There was a problem hiding this comment.
So I refactored a bit to have a GetTracer() function that returns a tracer. There's still one instance that gets initialized on first call, which isnt great but I'm not sure what better options there are. I could make a function + tracer instance like this for each package (then there wouldnt be a global one technically) but I dont see how that's any better. Let me know if you this this is acceptable, if not let's discuss options.
There was a problem hiding this comment.
I remember going through a similar type of conversation with @andresuribe87 on something a while back and he convinced me to rip out all the (complex, ugly) singleton code and just make a struct that returns a unique thing, which worked for the 90% case.
I believe what you've done here is fine too, and we should be open to changing it should that assumption be false
There was a problem hiding this comment.
I could see doing that too. It would involve passing it around through a lot of different functions, which seems annoying. I'm going to merge this as is, but i'm not opposed to moving to that in the future.
…tracer (creating if needed)
This resolves the issues outlined in #132
Debugf/Infof/Warnflogrus calls have been updated to use fields and a static log messageanother addition not mentioned in that issue is a simple traffic generator in
impl/integrationtest/main.gowhich generates a DID, PUTs it and GETs it every 30 seconds.Sample PUT span: