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(logs): Changing the service name to metadata-logs to be provider independent #2610

Merged
merged 1 commit into from
Nov 18, 2022
Merged

fix(logs): Changing the service name to metadata-logs to be provider independent #2610

merged 1 commit into from
Nov 18, 2022

Conversation

rnishtala-sumo
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo commented Nov 14, 2022 •
edited
Loading

Description
  • Changing the service name to be independent of the logs provider
  • Changes to integration tests to use the new service name
Checklist
  • Changelog updated
Testing performed
  • Redeploy metadata-logs and fluentbit
  • Confirm logs and metrics are coming in

@github-actions github-actions bot added the documentation documentation label Nov 14, 2022
@rnishtala-sumo rnishtala-sumo marked this pull request as ready for review November 14, 2022 22:33
@rnishtala-sumo rnishtala-sumo requested a review from a team as a code owner November 14, 2022 22:33
@rnishtala-sumo rnishtala-sumo marked this pull request as draft November 14, 2022 22:34
sumo-drosiek
sumo-drosiek reviewed Nov 15, 2022
View reviewed changes
deploy/helm/sumologic/templates/chart-configmap.yaml Outdated
@@ -3,6 +3,6 @@ kind: ConfigMap
metadata:
name: sumologic-configmap
data:
fluentdLogs: {{ template "sumologic.metadata.name.logs.service" . }}
otelcolLogs: {{ template "sumologic.metadata.name.logs.service" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we name it logs, metadataLogs or logsMetadata?

I always felt that using specific component name was bad decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, maybe I'll go for metadataLogs

sumo-drosiek
sumo-drosiek reviewed Nov 15, 2022
View reviewed changes
deploy/helm/sumologic/templates/_helpers.tpl Outdated
@@ -412,15 +412,11 @@ helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
{{- end -}}

{{- define "sumologic.metadata.name.logs" -}}
{{- if eq .Values.sumologic.logs.metadata.provider "fluentd" -}}
{{ template "sumologic.metadata.name.fluentd" . }}-logs
{{- else if eq .Values.sumologic.logs.metadata.provider "otelcol" -}}
{{ template "sumologic.metadata.name.otelcol" . }}-logs
Copy link
Contributor

Choose a reason for hiding this comment

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

We should leave statefulset names as they are. AFAIK, After this change we can end with fluentd statefulset named otelcol

Copy link
Contributor Author

@rnishtala-sumo rnishtala-sumo Nov 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

what I was trying to do here was to use the otelcol statefulset regardless of the provider. Is this not what is expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, I see what you mean, we can't change the name of a StatefulSet on upgrade. I'll change the default provider for logs and metrics to otelcol.

@rnishtala-sumo rnishtala-sumo force-pushed the rnishtala-fluentd-otelcol branch 5 times, most recently from 503964d to c10063f Compare November 16, 2022 22:56
@rnishtala-sumo rnishtala-sumo marked this pull request as ready for review November 16, 2022 22:59
@rnishtala-sumo rnishtala-sumo force-pushed the rnishtala-fluentd-otelcol branch 3 times, most recently from 58d6e02 to 531e187 Compare November 17, 2022 04:22
swiatekm
swiatekm reviewed Nov 17, 2022
View reviewed changes
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
deploy/helm/sumologic/templates/_helpers.tpl Outdated Show resolved Hide resolved
deploy/helm/sumologic/values.yaml Outdated Show resolved Hide resolved
tests/integration/helm_default_installation_test.go Outdated Show resolved Hide resolved
@rnishtala-sumo rnishtala-sumo changed the title Configuring fluentbit to send data to otelcol fix(logs): Changing the service name for metadata logs to be provider independent Nov 17, 2022
@rnishtala-sumo rnishtala-sumo changed the title fix(logs): Changing the service name for metadata logs to be provider independent fix(logs): Changing the service name to *-metadata-logs to be provider independent Nov 17, 2022
@rnishtala-sumo rnishtala-sumo force-pushed the rnishtala-fluentd-otelcol branch 2 times, most recently from b889b2b to b813bae Compare November 17, 2022 20:49
@rnishtala-sumo rnishtala-sumo changed the title fix(logs): Changing the service name to *-metadata-logs to be provider independent fix(logs): Changing the service name to metadata-logs to be provider independent Nov 17, 2022
@rnishtala-sumo rnishtala-sumo force-pushed the rnishtala-fluentd-otelcol branch 3 times, most recently from 6a8bf41 to 452e29e Compare November 17, 2022 21:34
@rnishtala-sumo rnishtala-sumo mentioned this pull request Nov 18, 2022
Merged
3 tasks
@swiatekm swiatekm self-requested a review November 18, 2022 15:40
@rnishtala-sumo rnishtala-sumo merged commit 396843a into main Nov 18, 2022
@rnishtala-sumo rnishtala-sumo deleted the rnishtala-fluentd-otelcol branch November 18, 2022 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions GitHub Actions github-actions approved these changes

@swiatekm swiatekm swiatekm approved these changes

@sumo-drosiek sumo-drosiek Awaiting requested review from sumo-drosiek

Assignees
No one assigned
Labels
documentation documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rnishtala-sumo @sumo-drosiek @swiatekm