Internal:Fix environment variable for remote configuration polling interval #2967
Conversation
marcotc
force-pushed
the
fix-remote-env-var
branch
from
bfa8775 to
490d98b
Compare
marcotc
added
dev/internal
| # Tune remote configuration polling interval. | ||
| # | ||
| # @default `DD_REMOTE_CONFIGURATION_POLL_INTERVAL_SECONDS` environment variable, otherwise `5.0` seconds. | ||
| # @default `DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS` environment variable, otherwise `5.0` seconds. | ||
| # @return [Float] | ||
| # @!visibility private |
There was a problem hiding this comment.
Should we document here a stronger "don't tune this, this setting is only used for internal validation and changing for other use-cases it is unsupported" or something like that?
There was a problem hiding this comment.
Since this is only used from system tests and system tests only use ENV variables, we might get away by not exposing the setting at all.
We could read the ENV variable. here:
and if it not present default to5.0
That way, there is no need to document that is private we just make it private 😎
There was a problem hiding this comment.
That's a good point, although you lose the consistency of always going through the settings. I guess we could at least move such settings to an internal or even debug group, if we wanted to still keep them.
There was a problem hiding this comment.
I live having it go through settings for consistency. It will get better with configuration validation 😉.
I've added stronger wording for it being private.
| ENV_ENABLED = 'DD_REMOTE_CONFIGURATION_ENABLED' | ||
| ENV_POLL_INTERVAL_SECONDS = 'DD_REMOTE_CONFIGURATION_POLL_INTERVAL_SECONDS' | ||
| ENV_POLL_INTERVAL_SECONDS = 'DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS' |
There was a problem hiding this comment.
This makes this setting somewhat inconsistent with DD_REMOTE_CONFIGURATION_ENABLED. I've searched on github and it seems internally we have both DD_REMOTE_CONFIGURATION_ENABLED and DD_REMOTE_CONFIG_ENABLED...
Maybe worth raising with the remote config folks what they want to do about this?
There was a problem hiding this comment.
I just asked on their Slack channel
DD_REMOTE_CONFIGURATION_POLL_INTERVAL_SECONDSwas changed toDD_REMOTE_CONFIG_POLL_INTERVAL_SECONDS.Some new system-tests fail when enabled for Ruby because
DD_REMOTE_CONFIG_POLL_INTERVAL_SECONDSis not processed byddtrace, making the test timeout.Also, this PR marks this setting as internal, because we don't have a valid use case for tweaking remote configuration pooling rate outside of testing purposes.