-
Notifications
You must be signed in to change notification settings - Fork 84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
logging: add configurable log-level to startup params #173
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,9 +4,10 @@ import android.content.Context | |
import io.envoyproxy.envoymobile.engine.EnvoyEngine | ||
|
||
// Wrapper class that allows for easy calling of Envoy's JNI interface in native Java. | ||
class Envoy( | ||
class Envoy @JvmOverloads constructor( | ||
context: Context, | ||
config: String | ||
config: String, | ||
logLevel: String = "info" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about introducing an enum for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
) { | ||
|
||
// Dedicated thread for running this instance of Envoy. | ||
|
@@ -21,7 +22,7 @@ class Envoy( | |
load(context) | ||
|
||
runner = Thread(Runnable { | ||
EnvoyEngine.run(config.trim()) | ||
EnvoyEngine.run(config.trim(), logLevel) | ||
}) | ||
|
||
runner.start() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,5 +10,10 @@ | |
|
||
/// Create a new Envoy instance. The Envoy runner NSThread is started as part of instance | ||
/// initialization with the configuration provided. | ||
- (instancetype)initWithConfig:(NSString*)config; | ||
- (instancetype)initWithConfig:(NSString *)config; | ||
|
||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would also be nice to document what valid log levels are available here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree enums would be better. Tell you all what I'll open another PR with enums after I merge and re-test iOS. :) |
||
|
||
@end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just gonna sneak this in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It's Apple's convention and
Left
wasn't getting applied consistently by clang-format anyways.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opinion here, but for context it was
Left
because that's what upstream Envoy uses for C++