Update data capture to match path pattern #60

shashank11p · 2021-04-07T15:51:10Z

Update DataCapture to match path pattern and use repeated DataCapture in AgentConfig for multiple data capture rules.
This can be used to define custom data capture rules for specific paths.
For example: If a user does not want to capture any data only for a specific endpoint that matches a path pattern, they can set the fields in the data capture rule with this path pattern to false.

config.proto

ryanericson · 2021-04-07T20:13:59Z

config.proto

+
+// CustomDataCaptureEndpoint represents a custom data capture rule for an endpoint that matches
+// the given url pattern and host header
+message CustomDataCaptureEndpoint {


Not sure about calling it Endpoint. Rule is better I think? But I'm open to other names too.

ryanericson · 2021-04-07T20:16:20Z

config.proto

+message CustomDataCaptureEndpoint {
+
+    // data_capture describes the data being captured by instrumentation for this endpoint
+    DataCapture data_capture = 1;


It's nice to describe this in terms of agent's DataCapture config instead of just having a single flag like capture_payload. This has implication on what we need to propagate: hypertrace/specification#7. It will not just be a single 0-1 flag to propagate in the header. Can we propagate the DataCapture definition? Otherwise we can propagate a matched rule ID, which downstream modules can lookup its DataCapture config definition of. Maybe the latter is nicer.

I would avoid to mix capture and propagation here because that is overkill. Locally services know what to capture and I don't think upstream should override that.

ryanericson · 2021-04-07T20:27:22Z

config.proto

+    DataCapture data_capture = 1;
+
+    // url_pattern describes the url pattern to match for this endpoint
+    google.protobuf.StringValue url_pattern = 2;


We should move path_pattern and host_header to a separate message. Something like Match (there must be a better name) which can be oneof different ways of matching against a span. One of them being UrlPathMatch which contains these two. This way the match behavior is more extensible.

I'd rather not do that. What identify the capture is the matching criteria, so having same settings for many matching criteria sounds more like a microoptimization. I'd keep it as it is.

jcchavezs · 2021-04-07T21:01:10Z

config.proto

@@ -127,3 +130,17 @@ message JavaAgent {
    // filter_jar_paths is the list of path to filter jars, separated by `,`
    repeated google.protobuf.StringValue filter_jar_paths = 1;
 }
+
+// CustomDataCaptureEndpoint represents a custom data capture rule for an endpoint that matches


To be clear this overrides whatever you set in the DataCapture config? How do you resolve conflicting rules (e.g. 2 matching rules with different DataCapture)? All this must be specified here.

I'm thinking we shouldn't introduce a new top level message here at all. Instead we should move this into the existing DataCapture message. I propose something along the lines of:

message DataCapture { Message http_headers = 1; Message http_body = 2; Message rpc_metadata = 3; Message rpc_body = 4; google.protobuf.Int32Value body_max_size_bytes = 5; google.protobuf.StringValue path_match = 6; repeated google.protobuf.StringValue headers_match = 7; }

and then change

message AgentConfig { ... repeated DataCapture data_capture = 3; ... }

I know changing data_capture to a repeated field is a breaking change, but it's not a widely deployed config yet, and will solve a lot of complexity in terms of defaults, precedence etc.

If we do this then we will not have a default at all and only have rules for the matches that are given in config. What about when there is no rule given in config for a path?

The default config would be a rule which matches everything. For the case of no config, the default in the agent would be capture everything.

So what you are proposing @davexroth is that if user does not pass a value for DataCapture (meaning empty list) we match everything with certain values for {http|grpc}_{headers|body}. I am curious of what would happen (it's possible I think) if the user wants to disable data capture for all, I am inclined to say I would just not pass anything and not expect a default to override that. I don't feel like a repeated is intuitive.

I would go for keeping the simple config for data capture and then a repeated message, same as DataCapture but also adding a path_regex_matcher and header_regex_matcher.

@jcchavezs I think disabling all still works pretty well with repeated. If path_match and headers_match are unspecified it means it will match all spans so disabling all would be specifying all headers and body message fields to false and not specifying path_match and headers_match.

So to be clear, what we are saying is that if the user does not pass any data capture we apply the default (capture all on everything) and if the user wants to disable data capture for everything then they have to create a data capture rule with no regex and everything in off which in practice is a empty object (at least in go, zero values are false), I think that is more confusing.

I honestly would prefer to be explicit on everything so having one DefaultDataCapture (which user can set or coming from the defaults) and then SpecificDataCapture there passing a regex for either path or header is mandatory.

Can we get a simple example using this branch on how that would look like in code? I think that is the best to decide what option to take.

Back to the question about what if we have many matching, we can describe that the ordering matters so when there is a match all the rest is discarded. This means we need to have a "order" concept in the UI and probably a way to reorder them. BTW ordering is respected by proto (see https://developers.google.com/protocol-buffers/docs/proto3#specifying_field_rules)

Here is an example in javaagent hypertrace/javaagent#301

I'm still not sure about requiring two messages, one for DefaultDataCapture and one for SpecificDataCapture - as it's the same thing.

If there's a feeling that being explicit is required, we could have two fields, default_data_capture and data_capture even though this can still be achieved in one field, and I'm not sure if it really add that's much clarity, and adds more combinations to worry about - what happens if data_capturee is defined but default_data_capture isn't. Having two messages has the same problem.

Even with two fields or messages, the agent still need to define what the default behavior is if the default field/message isn't defined.

default_data_capture does not have to be defined by the user because the agents generate it, The spec defines what the default behavior is.

jcchavezs · 2021-04-07T21:02:10Z

config.proto

+
+// CustomDataCaptureEndpoint represents a custom data capture rule for an endpoint that matches
+// the given url pattern and host header
+message CustomDataCaptureEndpoint {


How about PerEndpointDataCapture?

jcchavezs · 2021-04-07T21:06:20Z

config.proto

+    DataCapture data_capture = 1;
+
+    // url_pattern describes the url pattern to match for this endpoint
+    google.protobuf.StringValue url_pattern = 2;


How will this work for RPC?

ENV_VARS.md

…ern to it

pavolloffay · 2021-04-09T10:58:30Z

config.proto

+    // The first rule in config that matches for a path should be used if multiple rules match.
+    // Therefore the default rule (if present) should be the last rule.
+    // If no default rule is given, the default behaviour should be to allow data capture for all elements
+    repeated DataCapture data_capture = 3;


I have mixed feelings from the above description. It's not vague per se, but not elegant either.

I would keep the original data_capture as the default one and add a new field e.g. endpoint_data_capture of a new type

message EndpointDataCapture{ DataCapture data_capture = 1; // endpoint // host // probably also the sampling config }

We will also need whitelisted headers in the config https://github.com/hypertrace/specification/blob/main/agent/tracestate.md#whitelisted-headers. This should probably go to DataCapture

Let's take allowlisting headers separately, for the purpose of core mode processing it's even ok for it to be not configurable at the moment. I've opened https://traceableai.atlassian.net/browse/ENG-8716 to track it though.

pavolloffay · 2021-04-13T10:44:17Z

Two examples of config

service_name: emojis
propagation_formats:
  - B3
  - TRACECONTEXT
 reporting:
   endpoint: traceable-agent.traceableai:4317/
   trace_reporter_type: OTLP
data_capture: // the default case
    http_headers: false
endpoint_data_capture: // the override for a single endpoint
   - data_capture: 
        http_body: false 
     endpoint: /foo/bar

A single

service_name: emojis
propagation_formats:
  - B3
  - TRACECONTEXT
 reporting:
   endpoint: traceable-agent.traceableai:4317/
   trace_reporter_type: OTLP
data_capture:
   - data_capture: // the override for a single endpoint
        http_body: false
      endpoint: /foo/bar
   - data_capture: // the dafault case
      http_headers: false

davexroth · 2021-04-13T15:02:50Z

It's a personal preference, but I think the combined case reads more intuitively.

With the first example, there's now two concepts a user needs to know - data_capture and endpoint_data_capture. How are these different? What's the implication of choosing one over the other? The concept of an endpoint isn't really something any of the agents have had to date.

Also, at an implementation level, now you have two types you need to pass around, meaning that all the code that deals with data capture needs to handle both cases, leading to potentially duplicated code and bugs where the developer doesn't realize they need to handle both types of data_capture configuration.

ryanericson · 2021-04-14T00:28:04Z

My preference is to just have repeated DataConfiguration. It's generic and therefore extensible in the future if we want to provide overriding data capture config based on something other than endpoint e.g. exit call, sql, etc.
I see it as the one place for data capture config, default, endpoint/url based or any kind of override in the future.

adding custom data capture endpoint

3cb7fb3

shashank11p requested review from pavolloffay and davexroth April 7, 2021 15:51

shashank11p mentioned this pull request Apr 7, 2021

Add service for dynamic config #61

Draft

pavolloffay reviewed Apr 7, 2021

View reviewed changes

config.proto Outdated Show resolved Hide resolved

ryanericson reviewed Apr 7, 2021

View reviewed changes

jcchavezs reviewed Apr 7, 2021

View reviewed changes

ENV_VARS.md Outdated Show resolved Hide resolved

using existing DataCapture by making it repeated and adding path_patt…

6a5d700

…ern to it

shashank11p requested review from jcchavezs and pavolloffay April 9, 2021 05:13

shashank11p changed the title ~~Add custom data capture endpoints~~ Update data capture to match path pattern Apr 9, 2021

pavolloffay reviewed Apr 9, 2021

View reviewed changes

pavolloffay self-requested a review April 13, 2021 10:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update data capture to match path pattern #60

Update data capture to match path pattern #60

shashank11p commented Apr 7, 2021 •

edited

Loading

ryanericson Apr 7, 2021

ryanericson Apr 7, 2021

jcchavezs Apr 7, 2021

ryanericson Apr 7, 2021

jcchavezs Apr 7, 2021

jcchavezs Apr 7, 2021

davexroth Apr 7, 2021 •

edited

Loading

shashank11p Apr 8, 2021

davexroth Apr 8, 2021

jcchavezs Apr 8, 2021

ryanericson Apr 9, 2021

jcchavezs Apr 9, 2021 •

edited

Loading

shashank11p Apr 9, 2021

davexroth Apr 10, 2021

pavolloffay Apr 12, 2021

jcchavezs Apr 7, 2021

jcchavezs Apr 7, 2021

pavolloffay Apr 9, 2021 •

edited

Loading

pavolloffay Apr 9, 2021

ryanericson Apr 12, 2021 •

edited

Loading

pavolloffay commented Apr 13, 2021 •

edited

Loading

davexroth commented Apr 13, 2021

ryanericson commented Apr 14, 2021

Update data capture to match path pattern #60

Are you sure you want to change the base?

Update data capture to match path pattern #60

Conversation

shashank11p commented Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davexroth Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcchavezs Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay Apr 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanericson Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

pavolloffay commented Apr 13, 2021 • edited Loading

davexroth commented Apr 13, 2021

ryanericson commented Apr 14, 2021

shashank11p commented Apr 7, 2021 •

edited

Loading

davexroth Apr 7, 2021 •

edited

Loading

jcchavezs Apr 9, 2021 •

edited

Loading

pavolloffay Apr 9, 2021 •

edited

Loading

ryanericson Apr 12, 2021 •

edited

Loading

pavolloffay commented Apr 13, 2021 •

edited

Loading