Skip collecting logs and events on failure via env var #239
Conversation
dturn
changed the title
There was a problem hiding this comment.
I think this is worth trying, but we should leave it undocumented for now. Making it an env var seems okay... it's kinda a similar setting to log levels, which use env vars too. Naming is tricky though, since the most obvious choices sound like they could be talking about the gem's logging. DISABLE_FETCHING_DEBUG_INFO=1, DISABLE_KUBE_DEBUG_INFO=1, FETCH_EVENTS_AND_LOGS_ON_FAILURE=0?
I'm also wondering if we should make the logs and events switches separate. For huge deployments like the ones that are running into problems, events are significantly more useful than logs... but at a guess they're more likely the culprits of the fetch slowness.
| def test_debug_message_with_no_debug_info | ||
| ENV['NO_DEBUG_INFO'] = 'true' | ||
| dummy = DummyResource.new | ||
| dummy.expects(:deploy_failed?).returns(true) |
There was a problem hiding this comment.
Instead of using a stub, let's make this settable the same way we have done for deploy_succeeded? (add an attribute and a write for that attribute)
| dummy = DummyResource.new | ||
| dummy.expects(:deploy_failed?).returns(true) | ||
|
|
||
| assert_match dummy.debug_message, |
There was a problem hiding this comment.
Let's also refute that it matches - Logs or - Events
| @@ -12,6 +12,7 @@ class KubernetesResource | |||
| TIMEOUT = 5.minutes | |||
| LOG_LINE_COUNT = 250 | |||
|
|
|||
| NO_DEBUG_INFO_MESSAGE = "Event and log collection is disabled by the NO_DEBUG_INFO env var." | |||
There was a problem hiding this comment.
Nit: let's make the env var string itself a constant too, since it's used in three places.
|
This is ready for a second look. |
| @@ -169,12 +173,16 @@ def debug_message | |||
| @events.each do |identifier, event_hashes| | |||
| event_hashes.each { |event| helpful_info << " [#{identifier}]\t#{event}" } | |||
| end | |||
| elsif ENV[DISABLE_FETCHING_EVENT_INFO] | |||
| helpful_info << DISABLED_EVENT_INFO_MESSAGE | |||
There was a problem hiding this comment.
We should probably indent this (and the log message) with " - " like we do with the actual info. Looks a bit out of place:
(I'm using test_deploy_result_logging_for_mixed_result_deploy to see that)
When a bunch (~10) of deployments fail, there can be a 5+ minute delay between the polling loop completing and the statsd metric for completion being emitted. #238 sped this up by collecting logs and events in parallel. This PR goes one more step and provides a way to skip this altogether.
Open question: Should this be an env var?