Skip to content

Update data capture to match path pattern #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
shashank11p wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update data capture to match path pattern #60

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3cb7fb3
adding custom data capture endpoint
shashank11p Apr 7, 2021
6a5d700
using existing DataCapture by making it repeated and adding path_patt…
shashank11p Apr 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ENV_VARS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ Agents can be configured using environment variables:
| HT_PROPAGATION_FORMATS | List the supported propagation formats e.g. `HT_PROPAGATION_FORMATS="B3,TRACECONTEXT"`. |
| HT_ENABLED | When `false`, disables the agent |
| HT_JAVAAGENT_FILTER_JAR_PATHS | Is the list of path to filter jars, separated by `,`. |
| HT_CUSTOM_DATA_CAPTURE_ENDPOINTS | List the custom endpoints with respective data capture rules. |
Comment thread
shashank11p marked this conversation as resolved.
Outdated
17 changes: 17 additions & 0 deletions config.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ message AgentConfig {

// resource_attributes map define the static list of resources which is configured on the tracer
map<string, string> resource_attributes = 7;

// custom_data_capture_endpoints list the custom endpoints with respective data capture rules
repeated CustomDataCaptureEndpoint custom_data_capture_endpoints = 8;
}

// Reporting covers the options related to the mechanics for sending data to the
Expand Down Expand Up @@ -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
Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@davexroth davexroth Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown

@davexroth davexroth Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@ryanericson ryanericson Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor Author

@shashank11p shashank11p Apr 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is an example in javaagent hypertrace/javaagent#301

Copy link
Copy Markdown

@davexroth davexroth Apr 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@pavolloffay pavolloffay Apr 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// the given url pattern and host header
message CustomDataCaptureEndpoint {
Copy link
Copy Markdown

@ryanericson ryanericson Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about PerEndpointDataCapture?


// data_capture describes the data being captured by instrumentation for this endpoint
DataCapture data_capture = 1;
Copy link
Copy Markdown

@ryanericson ryanericson Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// url_pattern describes the url pattern to match for this endpoint
google.protobuf.StringValue url_pattern = 2;
Comment thread
shashank11p marked this conversation as resolved.
Outdated
Copy link
Copy Markdown

@ryanericson ryanericson Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@jcchavezs jcchavezs Apr 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this work for RPC?


// host_header describes the host header to match for this endpoint
google.protobuf.StringValue host_header = 3;
}