-
Notifications
You must be signed in to change notification settings - Fork 31
feat: Add a rust plugin to include a custom error page with the traceid #259
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.
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
base: main
Are you sure you want to change the base?
feat: Add a rust plugin to include a custom error page with the traceid #259
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
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.
LGTM once comments addressed
| if let Some(w3c_trace) = self.get_http_request_header("traceparent") { | ||
| // Format: version-trace_id-parent_id-flags | ||
| let parts: Vec<&str> = w3c_trace.split('-').collect(); | ||
| if parts.len() >= 4 && parts[1].len() == 32 { |
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.
Should >= be == ? Also consider using a regex to match and extract the field, so that it can verify all chars are hex digits.
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.
Adjusted it to be ==, >= was for future-proofing, in case additional elements are added to the format, but == works too! Also, added a regex and the newlines. Thanks!
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.
LGTM mod comment. Thanks!
| // Try W3C Trace Context standard | ||
| if let Some(w3c_trace) = self.get_http_request_header("traceparent") { | ||
| // Use regex to match the entire traceparent value and extract trace ID | ||
| let re = Regex::new(r"^[0-9a-f]{2}-([0-9a-f]{32})-[0-9a-f]{16}-[0-9a-f]{2}$").unwrap(); |
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.
Regex::new is an expensive operation, since it compiles a regular expression for matching (see https://cloud.google.com/service-extensions/docs/plugin-best-practices#precompile). Can you move this operation to be performed once, during RootContext initialization, and then you can reuse the Regex object from HttpContexts? See https://github.com/GoogleCloudPlatform/service-extensions/blob/main/plugins/samples/regex_rewrite/plugin.rs for an example of 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 nice, good to know this. just this some changes hopefully it's fine now. thanks.
Adding a plugin in rust that surfaces traceID in custom error pages.
We're checking for both Google Cloud trace header and W3C header.
Also adding a BUILD file and a tests.textpb file.