-
Notifications
You must be signed in to change notification settings - Fork 65
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
Add traced datastore #209
Add traced datastore #209
Conversation
90cb93c
to
9568b48
Compare
9568b48
to
12ac85a
Compare
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.
Two nits, but this otherwise looks great!
// Put implements the ds.Datastore interface. | ||
func (t *Datastore) Put(ctx context.Context, key ds.Key, value []byte) (err error) { | ||
ctx, span := t.tracer.Start(ctx, "Put", otel.WithAttributes(attribute.String("key", key.String()))) | ||
defer span.End() |
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.
We're not tracing errors anywhere. We probably want to call span.RecordError(err)
for all errors here (it'll ignore nil errors).
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.
ah good point, I guess it also makes sense to set the status then:
// RecordError will record err as an exception span event for this span. An
// additional call to SetStatus is required if the Status of the Span should
// be set to Error, as this method does not change the Span status. If this
// span is not being recorded or err is nil then this method does nothing.
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.
Well, that API is bonkers. But ok.
// Put implements the ds.Datastore interface. | ||
func (t *Datastore) Put(ctx context.Context, key ds.Key, value []byte) (err error) { | ||
ctx, span := t.tracer.Start(ctx, "Put", otel.WithAttributes(attribute.String("key", key.String()))) | ||
defer span.End() |
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.
Well, that API is bonkers. But ok.
This pull request introduces the package
trace
that wraps a datastore and applies open telemetry tracing to all datastore interactions.