Skip to content
New issue

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

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

Already on GitHub? # to your account

feat: enable forwarder daemonset by default #313

Merged
merged 1 commit into from
Feb 5, 2025

Conversation

obs-gh-alexlew
Copy link
Contributor

No description provided.

@obs-gh-alexlew obs-gh-alexlew force-pushed the alew/enable-forwarder-by-default branch 4 times, most recently from b01892a to 832a679 Compare February 4, 2025 00:56
@@ -13,6 +13,7 @@ resource "helm_release" "observe-agent" {
{
observe_url = var.observe_url,
observe_token = var.observe_token,
trace_token = var.observe_token,
Copy link
Contributor

Choose a reason for hiding this comment

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

If trace token is now becoming part of default helm deployment, it needs to be defined in this file
https://github.com/observeinc/helm-charts/blob/main/integration/modules/deploy_helm/variables.tf

Not sure why its specifically showing the "observe" namespace error but my guess is that it has to do something with this variable (or default) not being defined. Terraform plan is probably borked and the tftests don't really do plan/apply verbose like regular terraform

@obs-gh-alexlew obs-gh-alexlew force-pushed the alew/enable-forwarder-by-default branch 5 times, most recently from 4804f9c to f4ae563 Compare February 4, 2025 22:15
Copy link
Collaborator

@obs-gh-mattcotter obs-gh-mattcotter left a comment

Choose a reason for hiding this comment

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

lgtm other than @obs-gh-nikhildua 's comment

@@ -2,7 +2,7 @@
This example shows how to inject additional OTEL Config to the Observe Agent(s) that are running in the deployments and daemonsets. The additional config can add new instances of components such as receivers, processors, and exporters as well as to define new pipelines using those components. In addition, new pipelines can also use existing components from the default config.

## Values.yaml
In the helm chart, there are 3 deployments of Observe Agent and 1 daemonset. Each of these can have additional config injected. Each service has a corresponding field `agent.config.<service>` where the additional config can be added. The value of this field is a valid OTEL configuration in yaml format. The example values.yaml provided shows where custom OTEL config can be added for each of the four services.
In the helm chart, there are 3 deployments of Observe Agent and 2 daemonset. Each of these can have additional config injected. Each service has a corresponding field `agent.config.<service>` where the additional config can be added. The value of this field is a valid OTEL configuration in yaml format. The example values.yaml provided shows where custom OTEL config can be added for each of the four services.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
In the helm chart, there are 3 deployments of Observe Agent and 2 daemonset. Each of these can have additional config injected. Each service has a corresponding field `agent.config.<service>` where the additional config can be added. The value of this field is a valid OTEL configuration in yaml format. The example values.yaml provided shows where custom OTEL config can be added for each of the four services.
In the helm chart, there are 3 deployments of Observe Agent and 2 daemonsets. Each of these can have additional config injected. Each service has a corresponding field `agent.config.<service>` where the additional config can be added. The value of this field is a valid OTEL configuration in yaml format. The example values.yaml provided shows where custom OTEL config can be added for each of the four services.

@@ -11,7 +11,7 @@
"node_taint.yaml")
def test_helm_correctness(apps_client, helm_config):
"""
Test to verify there are 3 deployments and 1 daemonset in the cluster.
Test to verify there are 3 deployments and 2 daemonset in the cluster.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Test to verify there are 3 deployments and 2 daemonset in the cluster.
Test to verify there are 3 deployments and 2 daemonsets in the cluster.

@@ -21,7 +21,7 @@ def test_helm_correctness(apps_client, helm_config):
# Retrieve the daemonsets in the specified namespace
print(f"Checking Daemonsets in namespace: {helm_config['namespace']}")
daemonsets = apps_client.list_namespaced_daemon_set(namespace=helm_config['namespace']).items
assert len(daemonsets) == 1, f"Expected 1 daemonset in namespace '{helm_config['namespace']}', but found {len(daemonsets)}"
assert len(daemonsets) == 2, f"Expected 2 daemonset in namespace '{helm_config['namespace']}', but found {len(daemonsets)}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
assert len(daemonsets) == 2, f"Expected 2 daemonset in namespace '{helm_config['namespace']}', but found {len(daemonsets)}"
assert len(daemonsets) == 2, f"Expected 2 daemonsets in namespace '{helm_config['namespace']}', but found {len(daemonsets)}"

@obs-gh-alexlew obs-gh-alexlew force-pushed the alew/enable-forwarder-by-default branch from f4ae563 to cb202a9 Compare February 4, 2025 23:36
@obs-gh-alexlew obs-gh-alexlew force-pushed the alew/enable-forwarder-by-default branch from cb202a9 to 1e00dc5 Compare February 5, 2025 00:09
@obs-gh-alexlew obs-gh-alexlew merged commit f5f46d0 into main Feb 5, 2025
11 checks passed
@obs-gh-alexlew obs-gh-alexlew deleted the alew/enable-forwarder-by-default branch February 5, 2025 17:22
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants