Converted all logs to macros and modified existing log macros. by mrice32 · Pull Request #1068 · envoyproxy/envoy

mrice32 · 2017-06-08T20:33:05Z

Three types of logging: facility, general, and to a specific logger that the user passes in. I attempted to clean up the way that log macros were being done to make them a bit simpler. They should all filter down to the log_[level] macros for ease of compiling out certain log levels and global modification of log formatting or functionality.

mrice32 · 2017-06-08T20:34:36Z

@htuch @jamessynge

mrice32 · 2017-06-08T20:53:15Z

The CI is failing because envoy-filter-example is using the old logging macros. Are these macros considered a breaking change?

mattklein123 · 2017-06-08T21:26:34Z

The CI is failing because envoy-filter-example is using the old logging macros. Are these macros considered a breaking change?

Hmm. My inclination is to say to just go for it, but this breaking change probably means we should do the back-compat dance. Is it hard in this case? Seems like it should be relatively easy to do? (Haven't looked at change yet).

mrice32 · 2017-06-08T21:51:15Z

@mattklein123 It won't be too bad for most of them because the names are almost all distinct. The only ones whose names were repurposed were log_trace and log_debug. I think the only way to keep these two compatible would be change the names of the new ones. That should be fine though.

mattklein123 · 2017-06-08T22:03:07Z

I'm fine with making a breaking change personally, but it's going to be very hard to stage with the consuming project, and in some sense I think that's a good gut check that we shouldn't break stuff. So I guess maybe rename? @htuch ?

htuch · 2017-06-09T14:46:46Z

As discussed IRL, we can avoid the breaking change since it's not that bad to maintain compatibility, but certainly deprecate the old way.

htuch

Looks good, but some potato potatah stuff.

htuch · 2017-06-09T14:49:37Z

source/common/common/logger.h

 */
-#define conn_log(LOG, LEVEL, FORMAT, CONNECTION, ...)                                              \
-  LOG.LEVEL("[C{}] " FORMAT, (CONNECTION).id(), ##__VA_ARGS__)
+#define log_facility(LEVEL, ...) log_##LEVEL(log(), ##__VA_ARGS__)


I'm not a huge fan of the _facility suffix here, seems verbose and unclear what a "facility" is. Maybe we could add some explicit documentation of what a facility is here and/or rename to something (I'm not smart enough to figure out what that something should be).

Totally agree. This name was due to many name collisions (outside of just the Loggable class) with the macro log. I was also tossing around class_log (also ambiguous and verbose) or lg (which was inconsistent with the other names, but was nice in that it kept the preferred macro short). For now, I can add some documentation for what a facility is, and I'll ponder on the name.

htuch · 2017-06-09T14:54:57Z

source/common/common/logger.h

+ * Convenience macros to log to the general logger.
+ */
+#define get_general_logger() Logger::Loggable<Logger::Id::general>::log()
+#define log_general(LEVEL, ...) log_to_logger(get_general_logger(), LEVEL, ##__VA_ARGS__)


Should this be the general logger or misc logger. I think if it was called misc it would cause folks to think twice before using it when there is a reasonable way to put this in a "facility".

Good point. I think misc is a good way to discourage it.

…d macros.

mrice32 · 2017-06-12T19:42:35Z

@mattklein123 As discussed in #1088, I added a define for NVLOG (I'm happy to change the name). Its absence (the default) turns on verbose logging.

mattklein123

Looks great. Thanks for doing this. Few small things.

mattklein123 · 2017-06-12T19:47:26Z

DEPRECATED.md

  `statsd_udp_ip_address`.
  * `HttpFilterConfigFactory` filter API has been deprecated in favor of `NamedHttpFilterConfigFactory`.
  * Config option `http_codec_options` has been deprecated and has been replaced with `http2_settings`.
+  * The following log macros have been deprecated: `log_trace`, `log_debug`, `conn_log`, `conn_log_info`, `conn_log_debug`, `conn_log_trace`, `stream_log`, `stream_log_info`, `stream_log_debug`, `stream_log_trace`.  For replacements, please see [logger.h](https://github.com/lyft/envoy/blob/master/source/common/common/logger.h).


nit: please break at around 100 cols.

mattklein123 · 2017-06-12T19:47:37Z

source/common/common/logger.h

  FUNCTION(client)               \
  FUNCTION(config)               \
  FUNCTION(connection)           \
+  FUNCTION(misc)              \


nit: \ indent

mattklein123 · 2017-06-12T19:48:24Z

source/common/common/logger.h

+ * Base logging macros.  It is expected that users will use the convenience macros below rather than
+ * invoke these directly.
+ */
+#ifdef NVLOG


Can we document this somewhere on the bazel readme page? (Basically what --copt to set, and what it does, useful for perf, etc.)

mattklein123 · 2017-06-12T19:51:47Z

source/common/common/logger.h


-#ifdef NDEBUG
+/**
+ * Base logging macros.  It is expected that users will use the convenience macros below rather than


Now that we have macros everywhere, do you want to add file/line info also? Should be easy.

P.S. I don't care one way or the other, but you had mentioned this previously. can also do in follow up later.

htuch · 2017-06-13T01:02:02Z

source/common/common/logger.h

+
+/**
+ * Convenience macro to log to the facility of the class in which the macro is invoked. Note:
+ * facilities are a way of grouping classes within envoy. A facility is specified by Id enum values


Nit: s/envoyEnvoy

htuch · 2017-06-13T01:05:49Z

source/common/common/logger.h

+ * listed above. The facility_log is enabled within a class when the class inherits from the
+ * Loggable class templated by the choice of facility.
+ */
+#define log_facility(LEVEL, ...) log_to_logger(log(), LEVEL, ##__VA_ARGS__)


Still don't really like the term facility, but struggling with alternative suggestions:

"service" <- too easy to confuse with other notions of service in Envoy.

"category" <- make me think of category theory.

"class/object" <- not necessarily coupled directly with class/object.

Just "log" <- already taken.

The other alternative is we switch to all caps, which we probably should with macros anyway, then we use LOG etc.

I'm out of ideas and I can probably live with log_facility.

What about utility or group? Here are some others: http://www.thesaurus.com/browse/convenience, http://www.thesaurus.com/browse/service, and http://www.thesaurus.com/browse/category

I would be fine with all caps since it's a macro if that makes people happy.

htuch · 2017-06-13T15:55:35Z

It's fine as is as long people don't start explicitly using those macros.

The [backtrace] search regexes required updating.

Description: add guide for local stats testing. Risk Level: low Docs Changes: added docs, built locally. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

**Description** This parses OpenAI Web Tool requests and obfuscation used in gpt-5. As a part of this, we re-record all the affected requests. **Related Issues/PRs (if applicable)** #1068 --------- Signed-off-by: Adrian Cole <adrian@tetrate.io>

**Description** This commit is a follow up on the initial tracing implementation. There was a bug in vcr tests where streaming mode override is wrongly taken place at upstream filter level, which resulted in buffering all events at the router filter as opposed to the reality that they are streamed (not necessarily on the event boundary, as it's a tcp dataframe level), and that is why previously already passing the tests despite not taking care of the event boundaries. Notably, this makes the tracing do * Support for all backends regardless of the schema vs previously only works for OpenAI backend. * Correctly handling all streaming events vs previously assuming all events are available in buffer which is not true in end to end. * Avoids unnecessary copies around the tracing; This utilizes the already-parsed data structures, so this makes the tracing zero-copy as long as the redaction is configured. --------- Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>

Converted all logs to macros and modified existing log macros.

41643eb

htuch reviewed Jun 9, 2017

View reviewed changes

Partially responded to htuch's comments. Added back and deprecated ol…

fe2dd7b

…d macros.

mrice32 mentioned this pull request Jun 12, 2017

Add option to compile trace/debug messages on release builds #1088

Closed

mrice32 added 2 commits June 12, 2017 14:31

New define for turning off verbose logging. On by default. Added tests.

541c4dc

correct release compiler error

471290b

mattklein123 reviewed Jun 12, 2017

View reviewed changes

Corrected for comments and added line numbers/filenames to log format.

2e19872

htuch reviewed Jun 13, 2017

View reviewed changes

Made macros more concise by using all caps

e716299

htuch approved these changes Jun 13, 2017

View reviewed changes

mattklein123 approved these changes Jun 13, 2017

View reviewed changes

htuch merged commit 18fcfb7 into envoyproxy:master Jun 13, 2017

htuch added a commit to htuch/envoy that referenced this pull request Jun 14, 2017

Fix stack_decode.py post envoyproxy#1068.

69b8190

The [backtrace] search regexes required updating.

htuch added a commit that referenced this pull request Jun 14, 2017

Fix stack_decode.py post #1068. (#1110)

ad4d878

The [backtrace] search regexes required updating.

mrice32 deleted the log_macros branch June 14, 2017 17:09

Conversation

mrice32 commented Jun 8, 2017

Uh oh!

mrice32 commented Jun 8, 2017

Uh oh!

mrice32 commented Jun 8, 2017

Uh oh!

mattklein123 commented Jun 8, 2017

Uh oh!

mrice32 commented Jun 8, 2017

Uh oh!

mattklein123 commented Jun 8, 2017

Uh oh!

htuch commented Jun 9, 2017

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrice32 commented Jun 12, 2017

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hennna Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Jun 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hennna Jun 13, 2017 •

edited

Loading