feat: update filter interface to process only based on spans#223
feat: update filter interface to process only based on spans#223ryanericson merged 4 commits intohypertrace:mainfrom
Conversation
| if dataCaptureConfig.RpcBody.Request.Value && | ||
| len(reqBody) > 0 && err == nil { | ||
| setTruncatedBodyAttribute("request", reqBody, int(dataCaptureConfig.BodyMaxSizeBytes.Value), span) | ||
| if dataCaptureConfig.RpcMetadata.Request.Value { |
There was a problem hiding this comment.
@ryanericson @puneet-traceable @tim-mwangi This reordering was required because we now only have a single method in filter interface and the scenario of blocking based on headers was failing since the only call to filter was done on body first.
I think we should make similar changes to all instrumentations so that our issue of allow rule not taking precedence over blocking rules also will be solved.
Or we can pass flowvariable to Evaluate method to specify if we're calling for header eval or body eval
There was a problem hiding this comment.
So libtraceable interface only has one method called Evaluate now? Do the conditions that had it split up into various methods no longer apply?
There was a problem hiding this comment.
Yes, libtraceable interface has only one method Evaluate now.
Do the conditions that had it split up into various methods no longer apply?
This I'm not really sure. I looked through the code and couldn't find any reason why we had separate calls (maybe for consistency across agents). In both the cases we call the delegate after the body capture. I'll test with actual apps over the weekend.
There was a problem hiding this comment.
@tim-mwangi I tested with both a net/http app and and a grpc app, both of them are blocking with and w/o body, testing the remaining instrumentations as well
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #223 +/- ##
==========================================
- Coverage 59.03% 58.77% -0.26%
==========================================
Files 55 55
Lines 2248 2205 -43
==========================================
- Hits 1327 1296 -31
+ Misses 861 850 -11
+ Partials 60 59 -1 ☔ View full report in Codecov by Sentry. |
|
|
||
| processingBody := body | ||
| if int(h.dataCaptureConfig.BodyMaxProcessingSizeBytes.Value) < len(body) { | ||
| processingBody = body[:h.dataCaptureConfig.BodyMaxProcessingSizeBytes.Value] |
There was a problem hiding this comment.
dataCaptureConfig.BodyMaxProcessingSizeBytes config is also becoming obsolete as we are using the body stored as span attribute for filter evaluation which uses BodyMaxSizeBytes
There was a problem hiding this comment.
Yes, I'm ok with that.
There was a problem hiding this comment.
Can you help open a ticket to deprecate body max processing size throughout? Thanks!
There was a problem hiding this comment.
3 participants
Description
Updating the filter interface to have only a single method,
Evaluate. And when calling evaluate, all the relevant attributes/headers are expected to be present as part of the spanChecklist: