Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix telemetry templates #47894

Merged
merged 1 commit into from
Nov 17, 2023
Merged

Fix telemetry templates #47894

merged 1 commit into from
Nov 17, 2023

Conversation

bochengchu
Copy link
Contributor

@bochengchu bochengchu commented Nov 16, 2023 •
edited
Loading

Please provide a description of this PR:

  • Rename the variables.
  • Fix an issue that: If there's config set for SD logs but no for SD metrics, neither defaultProvider nor Envoyfilter is created.

If both SD metrics & SD logs are default, set in defaultProvider.
If any of SD metrics or logs has custom config set, use EnvoyFilter.

@bochengchu bochengchu requested a review from a team as a code owner November 16, 2023 22:01
@bochengchu bochengchu changed the title Fix telemetry Fix telemetry templates Nov 16, 2023
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test labels Nov 16, 2023
@istio-testing
Copy link
Collaborator

Hi @bochengchu. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

howardjohn
howardjohn reviewed Nov 16, 2023
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 question, rest looks good.

manifests/charts/istio-control/istio-discovery/templates/configmap.yaml
{{- if or $prom $sdMetrics $sdLogs }}
defaultProviders:
{{- if or $prom $sdMetrics }}
metrics:
{{ if $prom }}- prometheus{{ end }}
{{ if $sdMetrics }}- stackdriver{{ end }}
{{ if and $sdMetrics $sdLogs }}- stackdriver{{ end }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need logs enabled to turn on metrics? And the opposite on L22

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semantics would be: only set defaultProviders if both metrics and logs are default.
If one of them is default and another is custom, say metrics is default and log has custom setting, and we write {{ if $sdMetrics }}, it will end up having both defaultProviders set and EnvoyFilters created.

Not sure if it is okay to have both, but I think having only one of them can reduce confusion.

@bochengchu bochengchu mentioned this pull request Nov 16, 2023
Merged
@zirain
Copy link
Member

zirain commented Nov 17, 2023

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Nov 17, 2023
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 17, 2023
@istio-testing istio-testing merged commit cd981f4 into istio:master Nov 17, 2023
@bochengchu bochengchu deleted the fix-telemetry branch November 17, 2023 17:29
@bochengchu bochengchu mentioned this pull request Dec 19, 2023
Merged
@bochengchu
Copy link
Contributor Author

/cherry-pick release-1.19

@istio-testing
Copy link
Collaborator

@bochengchu: only istio org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherry-pick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@zirain
Copy link
Member

zirain commented Dec 19, 2023

/cherry-pick release-1.19

@istio-testing
Copy link
Collaborator

@zirain: #47894 failed to apply on top of branch "release-1.19":

Applying: Fix telemetry templates
Using index info to reconstruct a base tree...
A	manifests/charts/istio-control/istio-discovery/templates/telemetryv2_1.20.yaml
A	manifests/charts/istio-control/istio-discovery/templates/telemetryv2_1.21.yaml
A	manifests/charts/istiod-remote/templates/telemetryv2_1.20.yaml
A	manifests/charts/istiod-remote/templates/telemetryv2_1.21.yaml
Falling back to patching base and 3-way merge...
Auto-merging manifests/charts/istiod-remote/templates/telemetryv2_1.17.yaml
Auto-merging manifests/charts/istiod-remote/templates/telemetryv2_1.16.yaml
CONFLICT (content): Merge conflict in manifests/charts/istiod-remote/templates/telemetryv2_1.16.yaml
Auto-merging manifests/charts/istio-control/istio-discovery/templates/telemetryv2_1.17.yaml
Auto-merging manifests/charts/istio-control/istio-discovery/templates/telemetryv2_1.16.yaml
CONFLICT (content): Merge conflict in manifests/charts/istio-control/istio-discovery/templates/telemetryv2_1.16.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix telemetry templates
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing
Copy link
Collaborator

@zirain: new issue created for failed cherrypick: #48436

In response to this:

/cherry-pick release-1.19

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@istio-testing istio-testing mentioned this pull request Dec 19, 2023
Closed
This was referenced Dec 19, 2023
Merged
Merged
@jacob-delgado
Copy link
Contributor

/cherry-pick release-1.20

@istio-testing istio-testing mentioned this pull request Dec 19, 2023
Merged
@istio-testing
Copy link
Collaborator

@jacob-delgado: new pull request created: #48444

In response to this:

/cherry-pick release-1.20

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bochengchu @istio-testing @zirain @jacob-delgado @howardjohn