-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instrument Otel #125
Instrument Otel #125
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments, looks good so far
Nice work |
Fly-by comment: I've like to see some docs about where the parent span comes from, env? header field? etc. Additionally, given the distributed nature of dqlite, how spans in dlite peers may be linked to each other. |
@dimaqq Great question! In summary, a About your second question, we can created nested spans that are all associated with the same root span. If we need to inject additional information via the HTTP headers we can make use of propagators. |
Benchmark Result
Current status
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, forgot to press the submit button.
Only a minor comment afterwards we are good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Louise! Thanks for this effort! Here is a first pass on this PR.
pkg/kine/server/create.go
Outdated
if err == ErrKeyExists { | ||
span.RecordError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be recorded as it is not a misbehavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is removed.
pkg/kine/server/create.go
Outdated
return &etcdserverpb.TxnResponse{ | ||
Header: txnHeader(rev), | ||
Succeeded: false, | ||
}, nil | ||
} else if err != nil { | ||
span.RecordError(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use defer for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So span.RecordError(err)
does not stop the span, it will also record just nil
if there is no error. Why would you like to use defer here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking so much better! LGTM! Thanks!
Description
This PR introduces otel tracing logs.
Traces
We collect traces on (get/count/create/update/delete/list/update/compact/dbsize), this includes various child traces where relevant.
When you run
/hack/jaeger.sh
and runk8s-dqlite
with--otel=true
, the traces are collected in a local jaeger instance athttp://localhost:16686
.Metrics
The metric counters (get/count/create/update/delete/list/update) are written to stdout every 30 seconds.
(Limitation: Jaeger can't accept these metrics)
@bschimke95
Existing metrics
Side-note: Our kine layer already tracks some metrics here:
http://127.0.0.1:9042/metrics
:See an example output here