Skip to content

logging: Add ENVOY_TAGGED_LOG macro to enable tagged logging#27882

Merged
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
ohadvano:tagged_log
Jun 14, 2023
Merged

logging: Add ENVOY_TAGGED_LOG macro to enable tagged logging#27882
mattklein123 merged 5 commits intoenvoyproxy:mainfrom
ohadvano:tagged_log

Conversation

@ohadvano
Copy link
Contributor

@ohadvano ohadvano commented Jun 8, 2023

Commit Message: logging: Add ENVOY_TAGGED_LOG macro to enable tagged logging
Additional Description: Further description below
Risk Level: low (new macro)
Testing: Unit tests
Docs Changes: None
Release Notes: None
Platform Specific Features: None

Example usage:

std::map<std::string, std::string> tags{{"key", "val"}, {"key2", "val2"}};
ENVOY_TAGGED_LOG(info, tags, "some_message {}", "val");

Expected output message: [Tags: "key":"val","key2":"val2"] some message val

The idea here is that after #27278 was introduced to enable flushing application logs in JSON format, if json_format is set, then the %j flag would extract the tags from the message flushed to the sink, so the [Tags: "key":"val","key2":"val2"] part will be removed from the message. Additionally, a new flag will be introduced that will append the tags to the JSON string, so eventually we will get the following JSON string (for example): {"Timestamp":"%t","Message":"%j","key":"val","key2":"val2"}

Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
Copy link
Contributor Author

ohadvano commented Jun 8, 2023

/assign @mattklein123

@ohadvano
Copy link
Contributor Author

ohadvano commented Jun 8, 2023

Opening this to initial review just to make sure this is on track before completing the implementation. @mattklein123 I'll appreciate your review on this. For context, this is part 2 in the implementation of #25959.

Signed-off-by: ohadvano <ohadvano@gmail.com>
@mattklein123
Copy link
Member

I think in general we would not want to pay any map costs in the case where the logs are disabled. Can you make sure this can all be done inline inside the macro and only constructed after the level, etc. decisions are made? Thanks.

/wait


#define ENVOY_TAGGED_LOG_TO_LOGGER(LOGGER, LEVEL, TAGS, FORMAT, ...) \
do { \
if (ENVOY_LOG_COMP_LEVEL(LOGGER, LEVEL)) { \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattklein123 I added this check for the logger level before doing the serialization and proceeding with the costlier operations - are there more checks I have missed that should be added?

Copy link
Member

Choose a reason for hiding this comment

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

In your example you construct the map outside of the macro. We would optimally like to construct the map inside the macro if possible. Can you try that out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure I understand you correctly - do you mean to move the code of serializeLogTags to inside the macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or have you meant the usage exmple of

std::map<std::string, std::string> tags{{"key", "val"}, {"key2", "val2"}};
ENVOY_TAGGED_LOG(info, tags, "some_message {}", "val");

The idea was that this map could have been pre-created and saved in some class member to be reused in multiple ENVOY_TAGGED_LOG calls.

Can you demonstrate an alternative usage example so I could better understand please?

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is I could imagine doing something like:

ENVOY_TAGGED_LOG(info, {make tags here somehow}, "some_message {}", "val");

I just want to make sure that works correctly also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best option I could find is wrapping the tags map in parentheses, for example:

ENVOY_TAGGED_LOG(info, (std::map<std::string, std::string>{{"key", "val"}}), "some_message {}", "val");

If you define some using for example using LogTagsMap = std::map<std::string, std::string> then it would look like ENVOY_TAGGED_LOG(info, (LogTagsMap{{"key", "val"}}), "some_message {}", "val");, which is a bit nicer.

It's a bit awkward, but it works.. wdyt?

Copy link
Contributor Author

@ohadvano ohadvano Jun 13, 2023

Choose a reason for hiding this comment

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

Added tests that demonstrate and validate the two ways to use the macro, the PR is ready for review

ohadvano added 2 commits June 13, 2023 09:15
Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: ohadvano <ohadvano@gmail.com>
@ohadvano
Copy link
Contributor Author

/retest

@ohadvano ohadvano requested a review from mattklein123 June 13, 2023 11:34
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit 61af060 into envoyproxy:main Jun 14, 2023
@ohadvano ohadvano deleted the tagged_log branch June 14, 2023 04:47
asheryerm pushed a commit to asheryerm/envoy that referenced this pull request Jul 5, 2023
…oxy#27882)

Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: asheryer <asheryer@amazon.com>
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
…oxy#27882)

Signed-off-by: ohadvano <ohadvano@gmail.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
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.

2 participants