feat: instrument with tracing #15
Conversation
| } | ||
| } | ||
| return nil | ||
| return bss.PersistWithTx(ctx, tx) |
docker-compose.yml
Outdated
|
|
||
| volumes: | ||
| timescaledb: | ||
| grafana-data: |
There was a problem hiding this comment.
Did you intend to include the grafana volume here?
There was a problem hiding this comment.
👍 Good stuff. We are in urgent need of telemetry so I'd like to have this in asap since we can starting gathering metrics with it.
I'd also like to have some more discussion around the comments I've left. If the result of that discussion results in things here needing to change I am happy to take that as a follow on.
| @@ -23,3 +30,87 @@ type APIWrapper struct { | |||
| func (aw *APIWrapper) Store() adt.Store { | |||
| return aw.store | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
A couple thoughts on the change: this is definitely an improvement, but I have some concerns --- some of which you already mentioned:
- Are we going to grow this as the api consumed by Visor grows? Or is there some alternative?
- Since we are wrapping the API would it be better to have it as a field instead of embedding?
- Would it be more appropriate to add API tracing to lotus instead? (as a follow on later)
- Should we include the arguments to these calls as tags on the Span?
There was a problem hiding this comment.
We'll need to grow this if we want complete tracing coverage (we may not) or we should attempt to get it into lotus instead. I'm not convinced we want every method covered, especially since we are concerned about performance.
Wrapping the api, making it unexported would help enforce some type safety.
We could include the arguments as tags. I don't know useful it would be in general so I avoided paying the marshalling cost of things like cids in this pass.
| @@ -6,6 +6,33 @@ A component of [**Sentinel**](https://github.com/filecoin-project/sentinel), a c | |||
| A **Visor** process collects _permanent_ Filecoin chain meterics from a [**Lotus**](https://github.com/filecoin-project/lotus/) daemon, and writes them to a [**TimescaleDB**](https://github.com/timescale/timescaledb) time-series and relational datastore. | |||
|
|
|||
|
|
|||
| ## Getting Started | |||
services/indexer/indexer.go
Outdated
| trace "go.opentelemetry.io/otel/api/trace" | ||
| "github.com/opentracing/opentracing-go" |
There was a problem hiding this comment.
The ORM we are using supports tracing out-of-the-box: https://pg.uptrace.dev/tracing/. Are there any advantages to using opencensus over opentelemetry?
|
Could we make use of opencensus tracing instead of opentracing? We make use of opencensus for both metrics and tracing in lotus and it would be ideal use just the one tool. |
Yep, no problem. I'll rework |
|
OpenTracing and OpenCensus merged to start OpenTelemetry which is what the pg package supports, so I'll rework for that. There are software bridges to OpenCensus if needed. |
|
PTAL Switched to using opentelemetry. It has a more awkward API (3 imports needed to add one tag to a span) but it operates very closely to the opentracing code so it was mostly mechanical changes. Bonus screenshot of a trace showing go-pg traces as well as visor ones
|
Adds tracing to primary indexer and processor functions, all lotus api calls and database persistence operations.