feat: Add a go plugin to include a custom error page with the traceid#282
feat: Add a go plugin to include a custom error page with the traceid#282mateustd-ciandt wants to merge 7 commits intoGoogleCloudPlatform:mainfrom
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
|
|
||
| func getErrorTemplate(statusCode, traceID string) []byte { | ||
| templateOnce.Do(func() { | ||
| errorTemplateBytes = []byte(errorTemplate) |
There was a problem hiding this comment.
is errorTemplateBytes needed? Since []byte(errorTemplate) already performs a copy of errorTemplate, how about just doing:
result = []byte(errorTemplate)
result = bytes.ReplaceAll(result, ...)
result = bytes.ReplaceAll(result, ...)
return result
| } | ||
|
|
||
| // Check for error status codes (4xx or 5xx) | ||
| if status[0] != '4' && status[0] != '5' { |
There was a problem hiding this comment.
These numeric checks expressed as character checks doesn't feel very clean to me. This method already parses status to an int later on, suggest moving that conversion earlier and performing the error status code checks on the int value.
There was a problem hiding this comment.
true, made it cleaner.
| {"Content-Type", "text/html; charset=utf-8"}, | ||
| } | ||
|
|
||
| statusCode, err := strconv.ParseUint(status, 10, 32) |
There was a problem hiding this comment.
shorten to strconv.Atoi?
| func extractTraceID() string { | ||
| // Try Google Cloud trace header first | ||
| if traceHeader, err := proxywasm.GetHttpRequestHeader("x-cloud-trace-context"); err == nil { | ||
| for i := 0; i < len(traceHeader); i++ { |
There was a problem hiding this comment.
Include comment that states the expected format of the trace ID, like the C++ example
| // Try Google Cloud trace header first | ||
| if traceHeader, err := proxywasm.GetHttpRequestHeader("x-cloud-trace-context"); err == nil { | ||
| for i := 0; i < len(traceHeader); i++ { | ||
| if traceHeader[i] == '/' { |
There was a problem hiding this comment.
Can you use traceHeader.Index("/") or strings.Cut(traceHeader, "/") to do this, rather than hand-coding a for loop
There was a problem hiding this comment.
using Cut now
|
|
||
| // Try W3C Trace Context | ||
| if w3cTrace, err := proxywasm.GetHttpRequestHeader("traceparent"); err == nil { | ||
| if len(w3cTrace) >= 55 && w3cTrace[2] == '-' { |
There was a problem hiding this comment.
Please use a regex to do this, similar to the C++ example
There was a problem hiding this comment.
using pre-compiled regex matching C++ implementation
2 participants
Adding a plugin in GO 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.
Using the one in C++ as the model.