feat(server): move otlp server data from runs to its own table #3028
Conversation
| if !traceDB.ShouldRetry() { | ||
| return true, "TraceDB is not retryable" | ||
| } | ||
|
|
||
| maxTracePollRetry := job.PollingProfile.Periodic.MaxTracePollRetry() | ||
| // we're done if we have the same amount of spans after polling or `maxTracePollRetry` times | ||
| log.Printf("[PollerExecutor] Test %s Run %d: Job count %d, max retries: %d", job.Test.ID, job.Run.ID, job.EnqueueCount(), maxTracePollRetry) | ||
| if job.EnqueueCount() == maxTracePollRetry { | ||
| if job.EnqueueCount() >= maxTracePollRetry { |
There was a problem hiding this comment.
there was a weird bug where a job got incorrectly requeued after hitting the max retries limit. Since the new EnqueueConunt was bigger than maxTracePollRetry and the check was for equality, it got forever stuck in a polling loop
| "trace_id" varchar not null primary key, | ||
| "tenant_id" uuid, | ||
| "trace" JSONB, | ||
| "created_at" timestamp NOT NULL DEFAULT NOW() |
There was a problem hiding this comment.
this field gets autofilled on create. the rows are always inserted. for updates we use a delete+insert approach so we'll always have the correct value.
I think this might be useful for deleting old records from time to time
server/model/traceid.go
Outdated
There was a problem hiding this comment.
the code in this file was not being used
server/model/traces_test.go
Outdated
There was a problem hiding this comment.
this file was merged with this server/traces/traces_test.go
| } | ||
|
|
||
| if run.State != test.RunStateAwaitingTrace { | ||
| // test is not waiting for trace, so we can completely ignore those as they might |
There was a problem hiding this comment.
test integrity is now safe, since the results are saved to another table. the ingestor has no impact on the runs table
| return FromSpanList(flattenSpans) | ||
| } | ||
|
|
||
| func FromSpanList(input []*v1.Span) Trace { |
There was a problem hiding this comment.
this was extracted as an exported function so it could be reused by the ingestor more easily
| ) | ||
|
|
||
| func TestTraces(t *testing.T) { |
There was a problem hiding this comment.
this was merged from server/model/traces_test.go
| ctx, | ||
| `INSERT INTO otlp_traces (tenant_id, trace_id, trace) VALUES ($1, $2, $3)`, | ||
| sqlutil.TenantID(ctx), | ||
| trace.ID.String(), |
There was a problem hiding this comment.
Philosophical question: should we rely on each vendor to have a unique TraceID?
There was a problem hiding this comment.
This is a great question. I can't think of a simple way to make this more unique and still be able to accurately query the table. I will make the PK include the tenant_id so it's at least unique among tenants. I guess at some point we'll need to relate to to something else, but maybe we should wait to have this problem and better understand the usecase behind it
There was a problem hiding this comment.
Adding the tenant ID to the PK makes it non-nullable, so it's no good. all the queries are using the tenant_id, so while this is not enforced on the DB level, it is at the app level, making it essentially the same
Co-authored-by: Daniel Baptista Dias <danielbdias@users.noreply.github.com>
schoren
force-pushed
the
otlp-server-db-table
branch
from
e3308be
to
d046ae4
Compare
This PR makes the OTLP server save data on its own table instead of directly updating the
runtable with the received spans.This avoids bugs caused by moving the queues from in memory to external.
There wasn't a good place to put the
tracesrepository needed, so I took the resources approach, moved theTraceout of themodelpackage and into thetracespackage, and added the repo there. So, there are a lot of files changes that are just change the reference to the newtraces.Traceand relatedChanges
model.Traceand related structs intotraces.Tracetraces.TraceRepositoryused to store and access otlp server db dataotlp.Ingestorto interface with thetraces.TraceRepositoryChecklist