implement new request_id_utils extension system#10018
implement new request_id_utils extension system#10018rossdylan wants to merge 6 commits intoenvoyproxy:masterfrom rossdylan:request-id-utils
Conversation
There was a problem hiding this comment.
It doesn't look like proto is passed to this factory in any way.
There was a problem hiding this comment.
nit: maybe move this and other constants below to anonymous namespace in .cc file?
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
At a high level this LGTM. I put some high level comments around configuration and extension loading, so let's nail those first along with the other comments from @euroelessar and then I will take another pass. Note also that this will need release notes and some documentation, at least in https://www.envoyproxy.io/docs/envoy/latest/extending/extending.
Thank you for working on this!
/wait
There was a problem hiding this comment.
We should use a typed_config here (with any Any param) to allow for future configuration. This is how are we standardizing on loading extensions now. Please see other examples.
There was a problem hiding this comment.
Should the existing functionality be always linked with possible overriding? This will avoid build changes for all consumers that might rely on this functionality?
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
Bot
added
the
stale
stale
Bot
removed
the
stale
…in issue #7624. Signed-off-by: Ross Delinger <rossd@dropbox.com>
Signed-off-by: Ross Delinger <rossd@dropbox.com>
Signed-off-by: Ross Delinger <rossd@dropbox.com>
Signed-off-by: Ross Delinger <rossd@dropbox.com>
Signed-off-by: Ross Delinger <rossd@dropbox.com>
Signed-off-by: Ross Delinger <rossd@dropbox.com>
|
@rossdylan in the future friendly request to not force push. Please just merge master and add commits as it makes reviews easier. Thank you! |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks flushing out a few API comments. Excited to see this being worked on!
/wait
| } | ||
|
|
||
| message RequestIDUtils { | ||
| reserved 3; |
There was a problem hiding this comment.
I think this can be removed?
| string name = 1 [(validate.rules).string = {min_bytes: 1}]; | ||
|
|
||
| // Request ID utility specific configuration. | ||
| oneof config_type { | ||
| google.protobuf.Struct config = 2 [deprecated = true]; |
There was a problem hiding this comment.
Please remove both name and config, and just use typed_config to load the extension.
| // The configuration of the extension to use for request ID utilities. This includes operations such as | ||
| // generation, validation, and associated tracing operations. |
There was a problem hiding this comment.
Per my previous comment, I think if nothing is specified here we should just use the default implementation that is provided today? Can you clarify that?
There was a problem hiding this comment.
yeah I can clarify that, the current behaviour is that we use the UUIDUtils extension by default if nothing is provided. It is also linked in by default now.
|
This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
stale
Bot
added
the
stale
|
I'm taking over this pull request in #10429, please close this one & let's continue discussion in another one (or let me know if there is a better way to commandeer changes at GitHub if there is any) |
stale
Bot
removed
the
stale
3 participants
Description:
Allows for extensions to customize the behaviour of Request ID manipulation. This replaces the UUIDUtils class and makes it generic. This makes it possible to customize how request IDs are used for tracing and logging. This is my first contribution to envoy and it turned out to be a bit more invasive than I expected so I'd appreciate feedback on how I laid things out. I mostly followed the example of access_loggers but I'm not sure if that is the ideal.
Risk Level: Medium/High. No behaviour is changing, but UUIDUtils was wired into a bunch of different systems and extensions.
Testing: This PR is a work in progress, I still need to add new tests for the new code, but all existing tests pass with the changes.
Docs Changes:
Release Notes:
Implements changes from #7624