-
Notifications
You must be signed in to change notification settings - Fork 8
ENGTAI-63552: adding filter eval for client spans #250
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #250 +/- ##
==========================================
+ Coverage 47.79% 48.09% +0.29%
==========================================
Files 62 64 +2
Lines 3013 3144 +131
==========================================
+ Hits 1440 1512 +72
- Misses 1499 1553 +54
- Partials 74 79 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@@ -58,6 +62,29 @@ func (rt *roundTripper) RoundTrip(req *http.Request) (*http.Response, error) { | |||
req.Body = io.NopCloser(bytes.NewBuffer(body)) | |||
} | |||
|
|||
filterResult := rt.filter.Evaluate(span) | |||
if filterResult.Block { |
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.
is this for blocking client requests?
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.
Yes, since we are anyways calling libtraceable, thought we can add blocking support as well
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.
Hmm, I guess I'm wondering on use case, I understand that because we want to add sampling, we could add blocking support.
I think question is if we should.
This seems like it introduces ambiguity in our blocking feature as a whole. Up to this point, blocking has specifically been about preventing bad requests from making it into the system.
Since we don't currently propagate string taint from the original request to downstream client spans, it's unclear what would drive the decision to block here. The only scenarios I can think of are:
1.) The app loads something from a DB or config and decides to block the outbound request - which is totally unrelated to the original inbound request.
2.) We're trying to enforce some kind of policy based purely on app-generated or static context - which feels more like something the app should handle directly.
If we allow blocking here, we're essentially saying: "some internal logic or app state, something not tied to the inbound request caused us to block an outbound call."
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.
Agreed. But we also have DLP based blocking which is basically to prevent data leaks and was made for client spans only, but at that time none of our agents did blocking eval on client spans and hence user said they'll put all of their egress under a proxy and instrument our agent there so that DLP based blocking will work.
Also, I was thinking that even if we dont turn it on today, we can add the blocking capability in code and later when we want to turn it on, we can enable blocking for client spans as well, since we have a config on libtraceable, which by default skips client spans for blocking eval.
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.
But if you think even having the capability makes our feature ambiguous, then definitely removing 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.
Hmm, okay, but don't remove just because I don't think worth it -
i think worth identifying what we are trying to do..(I see you also did header injections) so is the purpose of these tickets to add sampling support, or add sampling support, header injections & blocking.
(my main intention was to ensure feature parity in the lang agents.., if we are adding these here, we need to make sure we do them in other lang agents)
I'd think this is a product question, if we see value in blocking client spans then sure..
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.
Thanks, lets start a thread with protection team/pm involved as well
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.
Sampling makes sense for client spans. Blocking I am not so sure. Let's get clarity from PM.
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.
I have started a thread here: https://harness.slack.com/archives/C08SEG313E3/p1751005003984389
We kind of discussed it back when rolling dlp out, but since its a huge lift, never prioritised it. I am unsure if we should prioritise it now as well, but I thought I can add it here since its not a lot of extra changes from adding sampling support to get it supported in goagent.
maybe 2 more questions: & similarly can db spans be sampled today? |
From a product perspective, I dont think dbclient spans blocking makes a lot of sense. Sampling still makes sense to me |
@varkey98 what's the ETA of this one? |
@tim-mwangi I'll try to wrap it up tomorrow my daytime |
Okay. Sounds good. |
@@ -6,7 +6,9 @@ toolchain go1.24.2 | |||
|
|||
replace github.com/hypertrace/goagent => ../.. | |||
|
|||
replace github.com/hypertrace/goagent/instrumentation/hypertrace/github.com/jackc/hyperpgx => ../../instrumentation/opentelemetry/github.com/jackc/hyperpgx | |||
replace github.com/hypertrace/goagent/instrumentation/hypertrace/github.com/jackc/hyperpgx => ../../instrumentation/hypertrace/github.com/jackc/hyperpgx |
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.
I need to check why we need this replace.
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.
one replace was always there pointing to the opentelemetry one's go module. When I added options pattern I added it only to the hypertrace wrapper (for other instrumentations like the grpc one we only had it for our hypertrace wrapper). So for using it I switched the replace from the otel one to hypertrace. But stranger part was that replace
was not working transitively, hence needed to add both.
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.
Hopefully it does not give me trouble during the migration :)
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.
yes, 🤞
Adding blocking and header injection support for http and grpc clients. For DB clients not sure about it from product side, hence just adding sampling support for now