-
Notifications
You must be signed in to change notification settings - Fork 183
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.
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
Fill cluster name property for traces #754
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{{- $yamlFile | replace "processors.source.collector.replace" $_collector | replace "exporters.zipkin.url_replace" $endpoint | replace "processors.source.name.replace" $sourceName | replace "processors.source.category.replace" $sourceCategory | replace "processors.source.category_prefix.replace" $sourceCategoryPrefix | replace "processors.source.category_replace_dash.replace" $sourceCategoryReplaceDash | replace "processors.source.exclude_namespace_regex.replace" $excludeNamespaceRegex | replace "processors.source.exclude_pod_regex.replace" $excludePodRegex | replace "processors.source.exclude_container_regex.replace" $excludeContainerRegex | replace "processors.source.exclude_host_regex.replace" $excludeHostRegex | replace "processors.resource.cluster.replace" $clusterName | nindent 2 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: split it on several lines to make reviews easier in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sumo-drosiek totally agree, tried to split into several lines but must have been doing it wrong and the chart never rendered; do you know the right way to do it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe something like that:
{{- $yamlFile = replace "processors.source.collector.replace" $_collector $yamlFIle }}
Description
cluster
property was not filled for traces previously. This fixes it, using the new tracing taxonomy (so it goes tok8s.cluster.name
)Testing performed