sam-heilbron commented 2 years ago

Description

Ensure that the stats server can handle requests to the /logging endpoint

Context

Issue: https://github.com/solo-io/go-utils/issues/475

Gloo Edge Context

The logging endpoint is a useful feature for debugging components. In fact, we even recommend this feature in our production debugging docs for Gloo Edge: https://docs.solo.io/gloo-edge/latest/operations/debugging_gloo/#changing-logging-levels-and-more

Problem

This feature has been broken for a while. You can see the history, but it has changed enough and been broken in different ways, that I decided to add a robust set of tests around the functionality.

In its most recent state, this handler is nil every time, unless the StartupOptions define a custom LogLevel.

Solution

I introduced a context aspect to this server, to ensure that we can shutdown the server when the parent context is cancelled. Without this functionality, consecutive tests would fail, because the listener would never be shutdown and we would return stale config.

How is this tested

I added a variety of unit tests to ensure that we can use the logging endpoint to return the current log level, given a variety of startup options.
I added a test to ensure that we can use the logging endpoint to change the log level, without requiring a restart to the server

I encountered some flakes, so to confirm that these tests don't introduce any flakes, I ran:

ginkgo -r -failFast -trace -progress -compilers=4 -failOnPending -noColor -randomizeSuites   -randomizeAllSpecs -untilItFails stats

and let it run for a while, before quitting

solo-changelog-bot[bot] commented 2 years ago

Issues linked to changelog: https://github.com/solo-io/gloo/issues/6449

jackstine commented 2 years ago

LGTM, I'll let Ethan approve it

solo-io / go-utils

Fix NPE in Stats Server Logging Handler #487