Skip to content

Add a cluster metadata access logger formatter#14911

Merged
yanavlasov merged 7 commits intoenvoyproxy:mainfrom
itamarkam:frmt
Feb 9, 2021
Merged

Add a cluster metadata access logger formatter#14911
yanavlasov merged 7 commits intoenvoyproxy:mainfrom
itamarkam:frmt

Conversation

@itamarkam
Copy link
Copy Markdown
Contributor

Commit Message: Add cluster metadata access logger formatter
Additional Description: Allows logging cluster metadata by using CLUSTER_METADATA(...)
Risk Level: Low (adding a new feature)
Testing: Unit tests
Docs Changes: Added documentation in docs/root/configuration/observability/access_log/usage.rst (very similar to the DYNAMIC_METADATA)
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

@omriz

@itamarkam
Copy link
Copy Markdown
Contributor Author

/assign yanavlasov

Copy link
Copy Markdown
Contributor Author

@itamarkam itamarkam Feb 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually not sure that this is not implemented for TCP, I just copied it from the dynamic metadata documentation.
I can understand why HTTP filter setting metadata would not be implemented for TCP for the dynamic metadata.
For cluster metadata, however, the question is whether the stream info contains any metadata for TCP clusters - @yanavlasov do you know how do that works?

Copy link
Copy Markdown
Contributor

@yanavlasov yanavlasov Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This metadata is for filters but I do not know if this applies to the TCP proxy as well. I will ask someone to look at this PR who might know.
From documentation it looks like it would not be applicable to the TCP proxy.

@itamarkam
Copy link
Copy Markdown
Contributor Author

/review yanavlasov

yanavlasov
yanavlasov previously approved these changes Feb 5, 2021
@yanavlasov
Copy link
Copy Markdown
Contributor

@itamarkam please merge main. I think it should solve the issue with the "verify examples" check.

@yanavlasov
Copy link
Copy Markdown
Contributor

/wait

Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
Signed-off-by: Itamar Kaminski <itamark@google.com>
@itamarkam
Copy link
Copy Markdown
Contributor Author

@itamarkam please merge main. I think it should solve the issue with the "verify examples" check.

Done.

@yanavlasov yanavlasov merged commit efcb0dc into envoyproxy:main Feb 9, 2021
@itamarkam itamarkam deleted the frmt branch February 9, 2021 14:09
@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Feb 9, 2021

@yanavlasov @itamarkam should we start implementing these formatters as extensions in the future (see #14512)? We could even transform this one into an extension (happy to volunteer), to ensure the list of core formatters doesn't keep growing.

@rgs1
Copy link
Copy Markdown
Member

rgs1 commented Feb 9, 2021

FWIW, I bumped into this change because I still carry a patch internally for #12991 while I move all our clusters into using that formatter implemented as an extension... so I had to rebase my in-house patch after this one (not a big deal, but another reason to use extensions!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants