feat(router): template expressions for access logs #1612
+1,839
−111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ssion # Conflicts: # router-tests/telemetry/telemetry_test.go
…he authentication code has been executed, and the other expressions as soon as possible
…s checks also auth usage in functions arguments
…tes-by-expression' into ale/eng-6258-custom-otel-attributes-by-expression
…s checks also auth usage in functions arguments
Router image scan passed✅ No security vulnerabilities found in image: |
endigma
reviewed
Feb 20, 2025
endigma
reviewed
Feb 20, 2025
SkArchon
force-pushed
the
milinda/eng-6254-add-the-capability-to-add-a-custom-log-field-through
branch
from
51ab90c to
58d494e
Compare
alepane21
approved these changes
Feb 21, 2025
Base automatically changed from
ale/eng-6258-custom-otel-attributes-by-expression
to
main
February 21, 2025 13:50
endigma
requested changes
Feb 21, 2025
Comment on lines
+265
to
+280
| func (c *requestContext) ResolveAnyExpressionWithWrappedError(expression *vm.Program) (any, error) { | ||
| // If an error exists already, wrap it and resolve the expression with the copied context | ||
| currExprContext := c.expressionContext.Request | ||
| if currExprContext.Error != nil { | ||
| copyExprContext := expr.Context{ | ||
| Request: expr.Request{ | ||
| Auth: currExprContext.Auth, | ||
| URL: currExprContext.URL, | ||
| Header: currExprContext.Header, | ||
| Error: &ExprWrapError{currExprContext.Error}, | ||
| }, | ||
| } | ||
| return expr.ResolveAnyExpression(expression, copyExprContext) | ||
| } | ||
| return expr.ResolveAnyExpression(expression, c.expressionContext) | ||
| } |
There was a problem hiding this comment.
I suggest flipping the if conditional here and returning early in the simple case, to reduce nesting.
Comment on lines
+282
to
+284
| func (c *requestContext) ResolveAnyExpression(expression *vm.Program) (any, error) { | ||
| return expr.ResolveAnyExpression(expression, c.expressionContext) | ||
| } |
There was a problem hiding this comment.
This function seems unused?
| } | ||
|
|
||
| func GetAccessLogConfigExpressions(attributes []config.CustomAttribute) ([]ExpressionAttribute, error) { | ||
| exprSlice := make([]ExpressionAttribute, 0) |
There was a problem hiding this comment.
Suggested change
| exprSlice := make([]ExpressionAttribute, 0) | |
| exprSlice := make([]ExpressionAttribute, 0, len(attributes)) |
Comment on lines
+42
to
+52
| func CleanupExpressionAttributes(attributes []config.CustomAttribute) []config.CustomAttribute { | ||
| filtered := make([]config.CustomAttribute, 0, len(attributes)) | ||
|
|
||
| for _, elem := range attributes { | ||
| if elem.ValueFrom == nil || elem.ValueFrom.Expression == "" { | ||
| filtered = append(filtered, elem) | ||
| } | ||
| } | ||
|
|
||
| return filtered | ||
| } |
There was a problem hiding this comment.
This function is untested in access_log_expression_test.go
endigma
reviewed
Feb 21, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
The goal is to extend the router to allow defining custom access log attributes by template expressions.
In order to do this the PR adds an "expression" field which can be set to process any log messages based on these exprlang expressions
However on top of this, the PR also goes ahead and adds the request
errorto theexpressionContext.This means that the user could now use expressions for access logs
This task was also previously mentioned in the PR: #1545 which was opened from a fork of mine, moved this to a branch in the repo so CI checks can run.
Checklist