Conversation
This makes the gRPC service log more then one ExecutionStream requests. The log level used will be controlled by the options associated to ExecutionStream requests. This change also incorporates a change to avoid creating multiple instances of Envoy::ProcessWide in the Nighthawk gRPC service flow. This doesn't seem to address any know issues, but it seems to be more along the way Envoy::ProcessWide was intended to be used. Fixes envoyproxy#289 Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
| return; | ||
| } | ||
|
|
||
| ProcessImpl process(*options, time_system_); |
There was a problem hiding this comment.
Note to reviewers: the fix is to not do this here. We do this once now at construction time, and hold on to the logging context throughout the lifetime of the ServiceImpl.
Note that ProcessImpl instantiations will change the log level per inbound configured options. We only allow one ProcessImpl instantiation at a time today, so this might be allright for now.
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
| : time_system_(time_system), stats_allocator_(symbol_table_), store_root_(stats_allocator_), | ||
| ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system, | ||
| const std::shared_ptr<Envoy::ProcessWide>& process_wide) | ||
| : process_wide_(process_wide == nullptr ? std::make_shared<Envoy::ProcessWide>() |
There was a problem hiding this comment.
Unsure about the context - do we need to worry about thread safety here or is ProcessImpl guaranteed to be constructed from a single thread?
There was a problem hiding this comment.
All ProcessImpl clients should ensure there's only a single active instance at any given time. We don't have to worry about it.
|
|
||
| ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system) | ||
| : time_system_(time_system), stats_allocator_(symbol_table_), store_root_(stats_allocator_), | ||
| ProcessImpl::ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system, |
There was a problem hiding this comment.
Is there any way how we can add test coverage for this change to keep the fixed functionality working?
There was a problem hiding this comment.
Actually, yes. Lacking C++ mocks for the logging, we can't easily unit-test this issue.
But I could add an end-to-end integration test for this, see 619c05e
| class ProcessImpl : public Process, public Envoy::Logger::Loggable<Envoy::Logger::Id::main> { | ||
| public: | ||
| ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system); | ||
| ProcessImpl(const Options& options, Envoy::Event::TimeSystem& time_system, |
There was a problem hiding this comment.
Can we add a doc comment? We could also explain the optional process_wide argument and say what happens if it isn't provided.
| public Envoy::Logger::Loggable<Envoy::Logger::Id::main> { | ||
|
|
||
| public: | ||
| ServiceImpl() : process_wide_(std::make_shared<Envoy::ProcessWide>()) { |
There was a problem hiding this comment.
Can we add a doc comment for the public constructor?
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
|
@mum4k thanks for the review; comments addressed! |
mum4k
left a comment
There was a problem hiding this comment.
Thank you. One remaining nit.
| assert ip_version != IpVersion.UNKNOWN | ||
| self.server_port = 0 | ||
| self.server_ip = server_ip | ||
| self.log_lines = None |
There was a problem hiding this comment.
(nit) Does this need to be public or would self._log_lines do? If public, can we document it above in the Attributes: section of the class docstring?
There was a problem hiding this comment.
Well, it gets consumed over at https://github.com/envoyproxy/nighthawk/pull/363/files#diff-9620177d1b84e578a841abdc925dcdf4R33. So it needs to be public, and documented. I'll add that.
There was a problem hiding this comment.
Done in 593fe3c (also moved the line that resets the log to None to a better place).
Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
This makes the gRPC service log more then one ExecutionStream requests.
The log level used will be controlled by the options associated to
ExecutionStream requests (possible candidate for improvement later on).
This change also incorporates a change to avoid creating multiple instances
of Envoy::ProcessWide in the Nighthawk gRPC service flow.
This doesn't seem to address any know issues, but it seems to be more along
the way Envoy::ProcessWide was intended to be used.
Fixes #289
Signed-off-by: Otto van der Schaaf oschaaf@we-amp.com