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.

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

chore(instrumentation): move instrumentation configuration from opentelemetry-operator #3733

Merged
merged 12 commits into from
Jun 13, 2024

Conversation

mat-rumian
Copy link
Contributor

@mat-rumian mat-rumian commented May 24, 2024

⚠️ BREAKING-CHANGE ⚠️

PR moves out instrumentation related configuration from opentelemetry-operator values.

Moved:

  • From opentelemetry-operator.instrumentationJobImage to instrumentation.instrumentationJobImage
  • From opentelemetry-operator.createDefaultInstrumentation to instrumentation.createDefaultInstrumentation
  • From opentelemetry-operator.instrumentationNamespaces to instrumentation.instrumentationNamespaces
  • From opentelemetry-operator.instrumentation.dotnet.traces to instrumentation.dotnet.traces
  • From opentelemetry-operator.instrumentation.dotnet.metrics to instrumentation.dotnet.metrics
  • From opentelemetry-operator.instrumentation.dotnet.extraEnvVars to instrumentation.dotnet.extraEnvVars
  • From opentelemetry-operator.instrumentation.java.traces to instrumentation.java.traces
  • From opentelemetry-operator.instrumentation.java.metrics to instrumentation.java.metrics
  • From opentelemetry-operator.instrumentation.java.extraEnvVars to instrumentation.java.extraEnvVars
  • From opentelemetry-operator.instrumentation.nodejs to instrumentation.nodejs
  • From opentelemetry-operator.instrumentation.python.traces to instrumentation.python.traces
  • From opentelemetry-operator.instrumentation.python.metrics to instrumentation.python.metrics
  • From opentelemetry-operator.instrumentation.python.extraEnvVars to instrumentation.python.extraEnvVars

Changed:

  • From opentelemetry-operator.instrumentation.dotnet.repository to opentelemetry-operator.autoInstrumentationImage.dotnet.repository
  • From opentelemetry-operator.instrumentation.dotnet.tag to opentelemetry-operator.autoInstrumentationImage.dotnet.tag
  • From opentelemetry-operator.instrumentation.java.repository to opentelemetry-operator.autoInstrumentationImage.java.repository
  • From opentelemetry-operator.instrumentation.java.tag to opentelemetry-operator.autoInstrumentationImage.java.tag
  • From opentelemetry-operator.instrumentation.nodejs.repository to opentelemetry-operator.autoInstrumentationImage.nodejs.repository
  • From opentelemetry-operator.instrumentation.nodejs.tag to opentelemetry-operator.autoInstrumentationImage.nodejs.tag
  • From opentelemetry-operator.instrumentation.python.repository to opentelemetry-operator.autoInstrumentationImage.python.repository
  • From opentelemetry-operator.instrumentation.python.tag to opentelemetry-operator.autoInstrumentationImage.python.tag

Deleted:

  • opentelemetry-operator.instrumentation.dotnet.image
  • opentelemetry-operator.instrumentation.java.image
  • opentelemetry-operator.instrumentation.nodejs.image
  • opentelemetry-operator.instrumentation.python.image

Checklist

  • Changelog updated or skip changelog label added
  • Documentation updated
  • Template tests added for new features
  • Integration tests added or modified for major features

@mat-rumian mat-rumian marked this pull request as ready for review May 27, 2024 14:40
@mat-rumian mat-rumian requested a review from a team as a code owner May 27, 2024 14:40
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This pull request contains invalid labels. Please remove all of the following labels: ['do-not-merge/hold']

@mat-rumian
Copy link
Contributor Author

Do not merge until PRs with documentation will be ready.

* From `opentelemetry-operator.instrumentation.python.metrics` to `instrumentation.python.metrics`
* From `opentelemetry-operator.instrumentation.python.extraEnvVars` to `instrumentation.python.extraEnvVars`

#### Changed:
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference between moved and changed?

Copy link
Contributor Author

@mat-rumian mat-rumian May 29, 2024

Choose a reason for hiding this comment

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

Moved are the keys moved from opentelemetry-operator key to instrumentation. Changed are keys which stay in the opentelemetry-operator but in different place (if it makes sense :))

Copy link
Contributor

Choose a reason for hiding this comment

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

It makes sense, but not sure if the distinguishment is significant and needed for customer

Copy link

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

The change looks good to me, I'm wondering how we should document it though. In practice, a lot of users unfortunately copy the entirety of our values.yaml, so this will block their upgrade even if they don't have tracing enabled.

I think we should:

  1. Add a section to the Troubleshooting documentation in help.sumologic.com specifically for this change.
  2. Try to figure out if we can show a custom error message pointing to that documentation.

@mat-rumian
Copy link
Contributor Author

mat-rumian commented Jun 11, 2024

@swiatekm-sumo

  1. Add a section to the Troubleshooting documentation in help.sumologic.com specifically for this change.

I've added a section in Troubleshooting documentation please see - SumoLogic/sumologic-documentation#4197

  1. Try to figure out if we can show a custom error message pointing to that documentation.

I was checking if it's possible but validation happens as first (even if I add some checks for specific keys in affected templates).

@swiatekm swiatekm self-requested a review June 11, 2024 15:34
@mat-rumian mat-rumian merged commit 79690d5 into main Jun 13, 2024
58 checks passed
@mat-rumian mat-rumian deleted the mat-rumian-move-instr-conf branch June 13, 2024 12:34
@sumo-drosiek sumo-drosiek mentioned this pull request Jul 1, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants