Add support for Elasticsearch client span - #801
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #801 +/- ##
==========================================
+ Coverage 55.06% 55.79% +0.72%
==========================================
Files 241 243 +2
Lines 20693 20822 +129
==========================================
+ Hits 11395 11618 +223
+ Misses 8540 8427 -113
- Partials 758 777 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
grcevski
left a comment
grcevski
left a comment
There was a problem hiding this comment.
Looks amazing!! I added some minor comments around testing certain functions and one bounds check. I'm approving and we are good to merge once these are addressed.
| return "" | ||
| } | ||
| parts := strings.Split(path, "/") | ||
| name := parts[len(parts)-1] |
There was a problem hiding this comment.
Let's be careful and check the len(parts) here.
| return httptest.NewRequest(method, target, strings.NewReader(body)) | ||
| } | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
Can we add tests for various malformed bodies and incomplete queries? It can happen with partial buffers and customers sending wrong requests.
There was a problem hiding this comment.
Hi @grcevski, I was implementing the required tests and it occurred to me: are we sure we want to consider a request with a malformed body/incomplete request as invalid?
I'm currently checking if the JSON is good or not and for example this "{" is an invalid json, while this "{"query": "not_object"}" is a valid json but not to be a valid request it should be for example like this "{"query":{"match_all":{}}}".
From our point of view it's just a request to an elasticsearch server which will reply with 400 Bad request and we can categorize it as elasticsearch request instead of just http.
What do you think?
There was a problem hiding this comment.
Oh that's a good point! let's keep it as is!
| } | ||
|
|
||
| if parseCtx != nil && !parseCtx.payloadExtraction.HTTP.GraphQL.Enabled { | ||
| if parseCtx != nil && !parseCtx.payloadExtraction.HTTP.GraphQL.Enabled && !parseCtx.payloadExtraction.HTTP.Elasticsearch.Enabled { |
There was a problem hiding this comment.
Do you mind making a helper function like any payload extraction enabled etc.?
Signed-off-by: Giuseppe Ognibene <giuseppe.ognibene@coralogix.com>
Signed-off-by: Giuseppe Ognibene <giuseppe.ognibene@coralogix.com>
165ef3b to
a99cfb9
Compare
|
Awesome stuff @pinoOgni ! |
Great work! Thanks for your contribution!!!
3 participants
This PR introduces support for Elasticsearch client spans as part of #141.
In this initial implementation, support has been added for the
_searchoperations. Additional operations will be covered in future PRs.