Fixes #23101 - add telemetry options #637
Conversation
manifests/init.pp
Outdated
| Boolean $telemetry_prometheus_enabled = $::foreman::params::telemetry_prometheus_enabled, | ||
| Boolean $telemetry_statsd_enabled = $::foreman::params::telemetry_statsd_enabled, | ||
| Optional[String] $telemetry_statsd_host = $::foreman::params::telemetry_statsd_host | ||
| Optional[Enum['statsd', 'statsite', 'datadog']] $telemetry_statsd_protocol = $::foreman::params::telemetry_statsd_protocol |
There was a problem hiding this comment.
I'd actually rather not add this, it can overwhelm users with options and we actually do not use any extensions, I think it might be better idea simply to remove this setting. Or at least write "keep statsd" in documentation comment.
There was a problem hiding this comment.
It's already hidden under advanced params. If we do not expose it and user changes protocol, on next upgrade, it would be overriden back. Not sure how you mean "keep statsd"
manifests/init.pp
Outdated
| Optional[String] $telemetry_statsd_host = $::foreman::params::telemetry_statsd_host | ||
| Optional[Enum['statsd', 'statsite', 'datadog']] $telemetry_statsd_protocol = $::foreman::params::telemetry_statsd_protocol | ||
| Boolean $telemetry_logger_enabled = $::foreman::params::telemetry_logger_enabled | ||
| Optional[Enum['DEBUG', 'INFO', 'WARN', 'ERROR', 'FATAL']] $telemetry_logger_level = $::foreman::params::telemetry_logger_level |
There was a problem hiding this comment.
This was meant to be for development, maybe at least mention this in docs comment. No practical use.
There was a problem hiding this comment.
the same as above, if we don't expose and people override, it would get reset every time installer runs, I could mention in docs if you suggest the text
manifests/init.pp
Outdated
| # $hsts_enabled:: Should HSTS enforcement in https requests be enabled | ||
| # $telemetry_statsd_host:: Statsd host in format ip:port, do not use DNS | ||
| # | ||
| # $telemetry_statsd_protocol:: Statsd protocol one of 'statsd', 'statsite' or 'datadog' |
There was a problem hiding this comment.
Please change to "Statsd protocol one of 'statsd', 'statsite' or 'datadog' - currently only statsd is supported
manifests/init.pp
Outdated
| # | ||
| # $telemetry_statsd_protocol:: Statsd protocol one of 'statsd', 'statsite' or 'datadog' | ||
| # | ||
| # $telemetry_logger_enabled:: Enable telemetry logs |
There was a problem hiding this comment.
"Enable telemetry logs - only useful for telemetry debugging"
manifests/init.pp
Outdated
| # | ||
| # $telemetry_logger_enabled:: Enable telemetry logs | ||
| # | ||
| # $telemetry_logger_level:: Telemetry logs level |
|
Updated parameters docs according to suggestions |
manifests/init.pp
Outdated
| @@ -303,6 +317,13 @@ | |||
| Enum['none', 'plain', 'login', 'cram-md5'] $email_smtp_authentication = $::foreman::params::email_smtp_authentication, | |||
| Optional[String] $email_smtp_user_name = $::foreman::params::email_smtp_user_name, | |||
| Optional[String] $email_smtp_password = $::foreman::params::email_smtp_password, | |||
| Optional[String] $telemetry_prefix = $::foreman::params::telemetry_prefix, | |||
There was a problem hiding this comment.
If there's a default, it can't be undef anymore so this can be String (technically users can do this via hiera, but let's ignore that). This is true for all the added parameters.
ares
force-pushed
the
fix/23101
branch
2 times, most recently
from
e5b7f75
to
398f632
Compare
manifests/init.pp
Outdated
| @@ -302,7 +316,14 @@ | |||
| Optional[String] $email_smtp_domain = $::foreman::params::email_smtp_domain, | |||
| Enum['none', 'plain', 'login', 'cram-md5'] $email_smtp_authentication = $::foreman::params::email_smtp_authentication, | |||
| Optional[String] $email_smtp_user_name = $::foreman::params::email_smtp_user_name, | |||
| Optional[String] $email_smtp_password = $::foreman::params::email_smtp_password, | |||
| String $email_smtp_password = $::foreman::params::email_smtp_password, | |||
There was a problem hiding this comment.
I think you didn't intend to change this line.
There was a problem hiding this comment.
no, certainly not, sorry :-)
theforeman-bot
added
Needs re-review
Needs testing
and removed
Waiting on contributor
labels
|
Doesn't this mean per default telemetry data will get send to localhost, no matter if anything is running there? |
|
ah, nope... it's only the configuration, it's still disabled 👍 |
|
meregd, díky @ares! |
@ekohl this would be really good to have in 1.18, otherwise every installer run disables telemetry configuration that users configured, cc @lzap
note that the biggest past is indentation of comments