logging: add configurable log-level to startup params #173
Conversation
Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: Mike Schore <[email protected]>
Signed-off-by: Mike Schore <[email protected]>
| context: Context, | ||
| config: String | ||
| config: String, | ||
| logLevel: String = "info" |
There was a problem hiding this comment.
What do you think about introducing an enum for this?
There was a problem hiding this comment.
I did think about it, and Envoy uses one internally.
However, currently we have to pass this as a string, and there's no portable enum type we could expose for this (so we'd have to define the enum on both platforms and still pass a string ultimately).
This seemed good enough for now to me. ¯\_(ツ)_/¯
There was a problem hiding this comment.
Yeah, I think we'll have to create one eventually. Let's keep that in mind on the next iteration for configurations/logs
There was a problem hiding this comment.
I definitely agree that this should be an enum at some point. What about making constants to use instead for now? Feels a little too much like a magic string
Signed-off-by: Mike Schore <[email protected]>
|
Looks good. What are your thoughts on having this be part of a configuration object in the future? Like bundling the log level and config in an object we pass into this layer. |
|
@buildbreaker I think bundling this into the configuration interface could definitely make sense depending on where we land with it. |
| @@ -26,7 +26,7 @@ IndentWidth: 2 | |||
| ObjCBlockIndentWidth: 2 | |||
| ObjCSpaceAfterProperty: true | |||
| ObjCSpaceBeforeProtocolList: true | |||
| PointerAlignment: Left | |||
| PointerAlignment: Right | |||
There was a problem hiding this comment.
Just gonna sneak this in here.
There was a problem hiding this comment.
(It's Apple's convention and Left wasn't getting applied consistently by clang-format anyways.)
There was a problem hiding this comment.
No opinion here, but for context it was Left because that's what upstream Envoy uses for C++
| @@ -26,7 +26,7 @@ IndentWidth: 2 | |||
| ObjCBlockIndentWidth: 2 | |||
| ObjCSpaceAfterProperty: true | |||
| ObjCSpaceBeforeProtocolList: true | |||
| PointerAlignment: Left | |||
| PointerAlignment: Right | |||
There was a problem hiding this comment.
No opinion here, but for context it was Left because that's what upstream Envoy uses for C++
| context: Context, | ||
| config: String | ||
| config: String, | ||
| logLevel: String = "info" |
There was a problem hiding this comment.
I definitely agree that this should be an enum at some point. What about making constants to use instead for now? Feels a little too much like a magic string
|
|
||
| /// Create a new Envoy instance. The Envoy runner NSThread is started as part of instance | ||
| /// initialization with the configuration provided. | ||
| - (instancetype)initWithConfig:(NSString *)config logLevel:(NSString *)logLevel; |
There was a problem hiding this comment.
Would also be nice to document what valid log levels are available here
There was a problem hiding this comment.
I agree enums would be better. Tell you all what I'll open another PR with enums after I merge and re-test iOS. :)
|
thank you @rebello95 and @buildbreaker! |
Signed-off-by: Jose Nino [email protected] Description: after #173 landed this target broke due to the breaking API change. This target will be added to CI once #181 closes, so breakages like this will not go undected. Risk Level: low - updating API, deleting old build rules.
Description: Adds configurable log-level to common interface, and support to both iOS and Android interfaces. Defaults to log-level info.
Risk Level: Low
Testing: Locally compiled and tested.