-
Notifications
You must be signed in to change notification settings - Fork 5.1k
access logging: support custom tag on ALS #18955
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
Conversation
Signed-off-by: Shikugawa <[email protected]>
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
…ustom-tag Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
/retest |
Retrying Azure Pipelines: |
/retest |
Retrying Azure Pipelines: |
@mathetake first pass? |
envoy/tracing/trace_driver.h
Outdated
@@ -5,6 +5,7 @@ | |||
#include <vector> | |||
|
|||
#include "envoy/common/pure.h" | |||
#include "envoy/data/accesslog/v3/accesslog.pb.h" |
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.
so this introduces the dependency on access_logs in tracing/trace_driver? I understand how this ends up here but seems a bit weird to me.
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.
I also considered this dependency seems weird, then submitted refactor PR. #18942 (comment)
But, maintainers referred to me utilizing the current API.
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.
huh I see... Thanks for putting the effort there!
Made drive-by comments :D |
Signed-off-by: Shikugawa <[email protected]>
@lizan Could you take a look? |
@lizan ping |
@lizan Could you help on this? |
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
/retest |
Retrying Azure Pipelines: |
@envoyproxy/envoy-maintainers Could anyone help 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.
Can you extract the custom tag refactor to another PR? otherwise looks good to me.
/wait |
Additional Description: This is part of #18955. Risk Level: Low Testing: CI Docs Changes: Release Notes: Platform Specific Features: Signed-off-by: Shikugawa <[email protected]>
Signed-off-by: Shikugawa <[email protected]>
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.
/retest
Retrying Azure Pipelines: |
/assign-from @envoyproxy/envoy-maintainers |
@envoyproxy/envoy-maintainers assignee is @snowp |
@snowp friendly ping |
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.
Thanks!
Great to see this gets merged. |
…19288) Additional Description: This is part of envoyproxy#18955. Risk Level: Low Testing: CI Docs Changes: Release Notes: Platform Specific Features: Signed-off-by: Shikugawa <[email protected]> Signed-off-by: Josh Perry <[email protected]>
Signed-off-by: Shikugawa <[email protected]> Signed-off-by: Josh Perry <[email protected]>
Signed-off-by: Shikugawa [email protected]
Commit Message: access logging: support custom tag on ALS
Additional Description: Close #18910. This PR adds custom tag feature that is similar to tracing span's. In this PR, we support this for ALS only.
Risk Level: Low
Testing: Unit
Docs Changes: Required
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]