Skip to content

Conversation

nicklewis
Copy link
Contributor

There is an undiagnosed bug around websocket logging on Windows, so
temporarily disable that logging until a solution can be found.

This restores the use of the default ASIO TLS client config, rather than
our custom config which enables devel logging. It also explicitly sets
the websocket log level to none and doesn't set a stream.

There is an undiagnosed bug around websocket logging on Windows, so
temporarily disable that logging until a solution can be found.

This restores the use of the default ASIO TLS client config, rather than
our custom config which enables devel logging. It also explicitly sets
the websocket log level to none and doesn't set a stream.
@nicklewis nicklewis requested a review from a team as a code owner December 2, 2021 23:11
@@ -85,10 +85,12 @@ Connection::Connection(std::vector<std::string> broker_ws_uris,
consecutive_pong_timeouts_ { 0 },
endpoint_ { new WS_Client_Type() }
{
// Disable websocket logging until PE-33165 is resolved.
setWebSocketLogLevel(leatherman::logging::log_level::none);
setWebSocketLogStream(nullptr);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, setWebSocketLogStream checks whether it's been given nullptr and just does nothing in that case. We could just delete this line, but I think it's helpful to have it explicit and make it clear which code needs to change to re-enable the logging.

@@ -58,7 +62,9 @@ static const std::string DEFAULT_CLOSE_REASON { "Closed by client" };

// Configuration of the WebSocket transport layer

using WS_Client_Type = websocketpp::client<ws_config>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use this for non-Windows platforms?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a slack conversation about looking in to that after this release gets out if we cant fix it for windows. https://puppet.slack.com/archives/CGJ0GTF4Y/p1638486827029700 for now we were thinking of just prioritizing a quick fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we probably can. We're reverting this for the puppet agent release that's supposed to ship in just a few days, so I want to wait until after that to experiment with selectively turning it on.

Copy link
Contributor

@steveax steveax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@steveax steveax merged commit 15b502f into puppetlabs:main Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants