Add javaagent config with filter jars#47
Conversation
|
Does it make sense to point a reference implementation for such opa
evaluator? Like us there any examples out there? Otherwise LGTM
…On Fri, 5 Feb 2021, 14:04 Shashank Patidar, ***@***.***> wrote:
------------------------------
You can view, comment on, or merge this pull request online at:
#47
Commit Summary
- adding javaagent config
File Changes
- *M* ENV_VARS.md
<https://github.com/hypertrace/agent-config/pull/47/files#diff-c241902f2164a724f98cd9373b625d9ae10ae1b2a76fb3d85f3ec3c85fa13aca>
(1)
- *M* config.proto
<https://github.com/hypertrace/agent-config/pull/47/files#diff-3eaf2c85e69e9ea4804954dd98f3b0e3de1047da36714aef929a41efac2fbd54>
(9)
Patch Links:
- https://github.com/hypertrace/agent-config/pull/47.patch
- https://github.com/hypertrace/agent-config/pull/47.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#47>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAXOYAU7SVRSBB56W4HLZUTS5PUEXANCNFSM4XEXNEFA>
.
|
| // JavaAgent has the configs specific to javaagent | ||
| message JavaAgent { | ||
| // opa_evaluator_jar_path is the path to opa evaluator jar | ||
| google.protobuf.StringValue opa_evaluator_jar_path = 1; |
There was a problem hiding this comment.
yep, it should be a list.
Also, it should be generic for any jar which implements the filter interface. Maybe rename to something like filter_jar_path.
There was a problem hiding this comment.
If this is a list we should clarify whether ordering is important, if all of them happen serial and if the result of one affect the others.
There was a problem hiding this comment.
This list will be used to load jars that implement filter interface, and it does not matter which order they are given. They will all be loaded and checked for blocking.. As per current logic in HT javaagent, If any one of the filter implementations returns true to block, the call will be blocked.
There was a problem hiding this comment.
If this is a list we should clarify whether ordering is important, if all of them happen serial and if the result of one affect the others.
+1
the semantics is as follows: block the request if any of the evaluators return true.
There was a problem hiding this comment.
that said the filters do not mutate state between each other, they should be totally independent.
There was a problem hiding this comment.
filters can add attributes right? I think it makes sense to define the order in which the filters run as the order they are defined here.
This would allow one fitter to add an attribute, and subsequent filters to change behavior based on those previously set attributes.
There was a problem hiding this comment.
This would allow one fitter to add an attribute, and subsequent filters to change behavior based on those previously set attributes.
The ordering is tricky since we use Java service loader to load filter provider implementations. The service loader does not guarantee the ordering. Though, we could work around it by creating a new classloader per filter jar.
I am not sure if we want to introduce coupling between filters, the other issue is that there is no read API on the span for attributes.
There was a problem hiding this comment.
+1 on having decoupled filters. Passing it in a config file sounds very fragile and not explicit. Could you elaborate on the exact use case @davexroth? Has other alternatives (like composing filters) being analyzed?
There was a problem hiding this comment.
Let's move this forward without explicit ordering. Ordering can be added later once we know why and how. Ordering won't be backwards incompatible.
Here is an implementation of the filter interface and an evaluator. |
pavolloffay
changed the title
|
|
||
| // JavaAgent has the configs specific to javaagent | ||
| message JavaAgent { | ||
| // filter_jar_paths is the list of path to filter jars, separated by `,` |
There was a problem hiding this comment.
separated by , is a env var detail so I would remove it.
4 participants
No description provided.