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

feat: change instrumentation related k8s objects #2647

Merged
merged 43 commits into from Nov 30, 2022
Merged

feat: change instrumentation related k8s objects #2647

merged 43 commits into from Nov 30, 2022

Conversation

mat-rumian
Copy link
Contributor

@mat-rumian mat-rumian commented Nov 25, 2022 •
edited

Description
  • rename otelagent to otelcol-instrumentation switch from daemonset to statefulset
  • rename otelcol to traces-sampler
  • rename otel-gateway to traces-gateway
  • moves processors like resource, source, resourcedetection from traces-sampler to otelcol-instrumentation
  • moves metrics exporter from traces-sampler to otelcol-instrumentation
  • cleanup services (deprecated ....otelcol and ....otelagent point to otelcol-instrumentation statefulset)
  • cleanup old traces values used in fluentd
  • enable port 8888 for otelcol-logs and otelcol-metrics by default
  • updates otel-distro version to 0.57.2-sumo-1
Checklist
  • Changelog updated
  • Helm tests updated
  • Integration tests updated
  • Cluster deployment tests (instrumentation flow)
  • Parameters list updated
Testing performed
  • Deployment on test cluster with enabled instrumentation
  • Deployment on test cluster with opentelemetry-operator enabled

@mat-rumian mat-rumian changed the title feat: change instrumentation related k8s objects WIP: feat: change instrumentation related k8s objects Nov 25, 2022
@mat-rumian mat-rumian marked this pull request as ready for review November 25, 2022 20:01
@mat-rumian mat-rumian requested a review from a team as a code owner November 25, 2022 20:01
swiatekm-sumo
swiatekm-sumo reviewed Nov 28, 2022
View reviewed changes
Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment •
edited

Choose a reason for hiding this comment

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

The changes themselves look fine to me. The two questions I have are:

  • are you going to add a config migration for the changes to values.yaml
  • and/or are you going to describe this in the migration guide

Seems like quite the change at first glance.

@mat-rumian mat-rumian changed the title WIP: feat: change instrumentation related k8s objects feat: change instrumentation related k8s objects Nov 29, 2022
sumo-drosiek
sumo-drosiek previously approved these changes Nov 30, 2022
View reviewed changes
@sumo-drosiek sumo-drosiek dismissed their stale review November 30, 2022 06:26

I saw only last commit

swiatekm-sumo
swiatekm-sumo approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@swiatekm-sumo swiatekm-sumo left a comment

Choose a reason for hiding this comment

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

As far as I can tell, this looks good, but I'd prefer if someone else reviewed it as well.

CHANGELOG.md Outdated Show resolved Hide resolved
@mat-rumian @swiatekm-sumo
Update CHANGELOG.md
8fa8e27 
Co-authored-by: Mikołaj Świątek <mswiatek@sumologic.com>
sumo-drosiek
sumo-drosiek reviewed Nov 30, 2022
View reviewed changes
deploy/helm/sumologic/templates/instrumentation/otelcol-instrumentation/statefulset.yaml Outdated
{{- if $otelcolInstrumentation.statefulset.extraEnvVars }}
{{- toYaml $otelcolInstrumentation.statefulset.extraEnvVars | nindent 8 }}
{{- end }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- end }}
{{- end }}

Copy link
Contributor

Choose a reason for hiding this comment

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

😂

sumo-drosiek
sumo-drosiek reviewed Nov 30, 2022
View reviewed changes
tests/helm/metadata_logs_fluentd/static/default_v2.output.yaml
Comment on lines +149 to +152




Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like it

@mat-rumian mat-rumian enabled auto-merge (squash) November 30, 2022 11:51
@mat-rumian mat-rumian merged commit b6c1ae0 into main Nov 30, 2022
@mat-rumian mat-rumian deleted the change-instrumentation-obj-arch branch November 30, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions approved these changes

@sumo-drosiek sumo-drosiek sumo-drosiek approved these changes

@swiatekm-sumo swiatekm-sumo swiatekm-sumo approved these changes

Assignees
No one assigned
Labels
breaking-change documentation documentation integration-tests priority:high
Projects
None yet
Milestone
v3.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mat-rumian @sumo-drosiek @swiatekm-sumo