-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 option to use android logger for process logs #9767
Conversation
Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
Signed-off-by: Jose Nino <[email protected]>
@jmarantz do you mind taking a look at this approach? My initial thought was to derive from the SinkDelegate class structure. However, I went for the lowest level at which I could interact with a spdlog::sinks::sink, and that was replacing the DelegatingLogSink whole sale when we construct Logger's logger_. This seemed cleaned enough, but do let me know if you can think of something else. Very open to suggestions. |
Signed-off-by: Jose Nino <[email protected]>
@zuercher do you mind taking a look? |
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.
I think this is generally fine, but I had a question about the bazel side of this.
"//conditions:default": [], | ||
}), | ||
strip_include_prefix = "android", | ||
deps = [":base_logger_lib"], |
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.
Given that the andoid sink in spdlog is #ifdef __ANDROID__
, does it make sense to gate this logger on the android equivalent of //bazel::windows_x86_64
and //bazel:apple
(instead of a flag that won't work in any other env) and following the pattern used for envoy_cc_win32_library
?
(The file watcher lib should get the same treatment for apple, but that's another PR.)
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.
That was my first idea, but @goaway pointed out that there might be cases where we might want the StandardLogger when __ANDROID__
is defined, e.g running envoy as a process in an android device, rather than in a sandboxed application.
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.
Ok. Do we want to pull the selection logic out into helpers?
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.
I also debated about this. Given that it was only used once now and for the foreseeable future I opted for not introducing more helpers into the build system until they are needed. But if you think it is worth it, I am happy to add!
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.
I think the thing I'm getting it is whether having an empty envoy_cc_library (logger_impl_lib_android) when android logging is disabled causes some kind of problem for bazel (particular the query subcommand). It seemed like there was something related to that with win32 libs, but I'm a bit out of my depth when it comes to bazel. I suppose it can always be fixed later.
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.
I see. I was copying the pattern from win32
Line 151 in b78fc4e
def envoy_cc_win32_library(name, srcs = [], hdrs = [], **kargs): |
Signed-off-by: Jose Nino <[email protected]>
Description: update the envoy ref. This bump includes envoyproxy/envoy#9767 which will enable us to turn on process logs for Android devices in a subsequent PR. Risk Level: med, as always with a ref update. Testing: CI Signed-off-by: Jose Nino <[email protected]>
Description: envoyproxy/envoy#9767 introduced the bazel option and #663 brought it into envoy mobile. This PR changes the bazel config for android to turn on the logger by default when building on android. It also updates a couple other stale flags and docs. Risk Level: low Testing: tested locally and saw logs Fixes #34 Signed-off-by: Jose Nino <[email protected]>
Description: update the envoy ref. This bump includes #9767 which will enable us to turn on process logs for Android devices in a subsequent PR. Risk Level: med, as always with a ref update. Testing: CI Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: #9767 introduced the bazel option and #663 brought it into envoy mobile. This PR changes the bazel config for android to turn on the logger by default when building on android. It also updates a couple other stale flags and docs. Risk Level: low Testing: tested locally and saw logs Fixes #34 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: update the envoy ref. This bump includes #9767 which will enable us to turn on process logs for Android devices in a subsequent PR. Risk Level: med, as always with a ref update. Testing: CI Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: #9767 introduced the bazel option and #663 brought it into envoy mobile. This PR changes the bazel config for android to turn on the logger by default when building on android. It also updates a couple other stale flags and docs. Risk Level: low Testing: tested locally and saw logs Fixes #34 Signed-off-by: Jose Nino <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: this PR adds an option to select different process loggers based on bazel config. Notably it introduces an Android Logger that is able to log to Android App's log stream.
Risk Level: low. The new functionality is bazel config guarded.
Testing: local. CI
Docs Changes: documented in bazel's README.md