-
Notifications
You must be signed in to change notification settings - Fork 438
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
elastic: Add traced HTTPClient for client #68
Conversation
I should mention that there wasn't a way to directly hook into the |
@@ -0,0 +1,98 @@ | |||
// Package elastictraced provides tracing for the Elastic Elasticsearch client (https://github.com/olivere/elastic) |
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.
Are we compatible only with v5
?
We should explicit the compatible versions in the doc (and if that's more than v5
, we should also add the others in the tests).
56b7e41
to
f057825
Compare
} | ||
} | ||
|
||
// pathIsTraced returns a boolean indicating if a given Elasticsearch path |
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.
What was the reasons behind these rules?
- I guess it is legit to get stats on health checks/internal checks? Are apps really going to hit these endpoints anyway?
- while we can't get much metadata, bulk requests can represent most of the operations for some workers?
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.
- The health check was coming from the elastic library itself, on startup. But fair, I can leave it in.
- My concern on bulk was adding overhead when reading the body but maybe we can not read it if it's greater than some size. I'll look into 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.
- Oh, I didn't know about this internal! When you say startup, it is the initial connection? After the first query? If that's spammy or outside of the context of a real traced transaction, I'm good with skipping it. Otherwise, it is good to see what's really happening during the traced code.
- Oh, good concern. From what I recalled, we even internally forked this client to avoid this expensive operation. If I recall well, the hack was to read it manually, the "error" was at the beginning of the Body so we were able to find it for cheap by just looking at the beginning of the body.
} | ||
res.Body = ioutil.NopCloser(bytes.NewBuffer(buf)) | ||
} | ||
span.SetMetric(ext.HTTPCode, float64(res.StatusCode)) |
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 might be a number, it is more legit to see it as a "code" ie. using SetMeta. "Metric" were more for graphable values (but we aren't sure we will keep that in the future).
res, err := t.Transport.RoundTrip(req) | ||
|
||
if err != nil { | ||
span.SetError(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.
Would res.StatusCode
be available here? Would be nice to set it as a meta too.
TracedHTTPClient hooks into the http.Transport to trace requests and capture spans for Elasticsearch. It's generic enough that it could be used for other Elasticsearch clients that allow you to override the http client. The tests and example show the pattern for tracing the elastic client, specifically using elastic.v5. Older versions do not support calling `Do` with a context so they won't apply. But in theory this should be compatible with new version going forward. docker-compose is updated to run ES for the integration tests.
f057825
to
1b89da3
Compare
TracedHTTPClient hooks into the http.Transport to trace requests and
capture spans for Elasticsearch. It's generic enough that it could be
used for other Elasticsearch clients that allow you to override the http
client.
The resource is quantized following the style used in dd-trace-py
The tests and example show the pattern for tracing the elastic client,
specifically using elastic.v5. Older versions do not support calling
Do
with acontext
so they won't apply. But in theory this should becompatible with new version going forward.
docker-compose is updated to run ES for the integration tests.