chore: add APP_INSIGHTS_ID to image build #1266
Conversation
Signed-off-by: Alex Castilio dos Santos <[email protected]>
alexcastilio
force-pushed
the
alexcastilio/app-insights-in-image
branch
3 times, most recently
from
fd1f6a8 to
e175158
Compare
alexcastilio
force-pushed
the
alexcastilio/app-insights-in-image
branch
from
e175158 to
585155c
Compare
Signed-off-by: Alex Castilio dos Santos <[email protected]>
alexcastilio
force-pushed
the
alexcastilio/app-insights-in-image
branch
from
585155c to
b82c68c
Compare
| @@ -35,5 +37,10 @@ func GetConfig(cfgFileName string) (*OperatorConfig, error) { | |||
| return nil, fmt.Errorf("error unmarshalling config: %w", err) | |||
| } | |||
|
|
|||
| // If unset, default telemetry interval to 5 minutes. | |||
| if cfg.TelemetryInterval == 0 { | |||
| cfg.TelemetryInterval = 5 * time.Minute | |||
There was a problem hiding this comment.
let's keep to 15min, and since this is a configmap change we'll need to notify a few people
There was a problem hiding this comment.
I think it would be better if a zero value TelemetryInterval means no telemetry/the previous behavior. That keeps any changed behavior intentionally opt-in, backcompat, and obvious
There was a problem hiding this comment.
@matmerr currently it is 15 min for the daemonset heartbeat. This config is for operator, which is 5 min. No change in default values:
https://github.com/microsoft/retina/pull/1266/files#diff-3707c2e5d521fb2df733befba56a5cf6044f7a0d08e8a2ffd1b8ce2a2351731dL59
@rbtr currently a different key in the yaml will enable telemetry:
Do you think enableTelemetry key should be removed and keep only the telemetryInterval? (disable telemetry if time is set to 0)
There was a problem hiding this comment.
@huntergregory is there a specific reason operator heartbeat is so frequent at 5min?
There was a problem hiding this comment.
I think it would be better if a zero value TelemetryInterval means no telemetry/the previous behavior. That keeps any changed behavior intentionally opt-in, backcompat, and obvious
@rbtr in this specific case I think reading telemetryInterval as 0 may require a user to jump to docs. I could see it being interpreted as disabled and don't send telemetry at all, or send telemetry in a continuous stream with a 0 second delay. I kinda prefer keeping the whole telemetry functionality behind the explicit enableTelemetry bool
There was a problem hiding this comment.
if there's a separate enable flag that bypasses this entirely, on objection. let's just make sure that if this is unset it defaults to the previous behavior
| @@ -9,3 +9,4 @@ metricsIntervalDuration: "10s" | |||
| # used to export telemetry to AppInsights | |||
| telemetryEnabled: true | |||
| dataAggregationLevel: "low" | |||
| telemetryInterval: "15m" | |||
There was a problem hiding this comment.
we can be careful, but is it possible to add a test around the behavior if someone just uses 15 instead of 15m?
Description
deploy/standard/manifests/controller/helm/retina/values.yamlRelated Issue
If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.
Checklist
git commit -S -s ...). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
Manual test was also done to validate that the configuration was propagated from helm chart
values.yamlfile to both operator and agent. Value set was printed to log:Additional Notes
Add any additional notes or context about the pull request here.
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.