-
Notifications
You must be signed in to change notification settings - Fork 652
Fix issue with resourcedetection processor failing with no detectors … #4392
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
base: main
Are you sure you want to change the base?
Conversation
| outConfig *Config, | ||
| ) { | ||
| var spec *v1beta1.InstrumentationLogsSpec | ||
| if inCluster != nil && inCluster.Spec.Instrumentation != nil { |
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.
❓ Did something change so that these checks are no longer necessary?
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.
So, we checked them to ensure that accessing inCluster.Spec.Instrumentation.Logs wouldn't result in a nil pointer error, but the inCluster.Spec.Instrumentation != nil check happens in the OpenTelemetryLogsEnabled function call below anyways, so we can get rid of them and move the spec assignment inside the if block... That being said, OpenTelemetryLogsEnabled doesn't check that inCluster != nil, so we actually theoretically could have had nil pointer exceptions even before my change, although I don't really know if this is possible since I can't think of a reason why we'd be passing around a nil pointer to a v1beta1.PostgresCluster... Either way, probably a good idea to stick a check in there.
| logfile string, | ||
| ) { | ||
| var spec *v1beta1.InstrumentationLogsSpec | ||
| if inCluster != nil && inCluster.Spec.Instrumentation != nil { |
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.
❓ Same question as above.
| outConfig *Config, | ||
| ) { | ||
| var spec *v1beta1.InstrumentationLogsSpec | ||
| if inCluster != nil && inCluster.Spec.Instrumentation != nil { |
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.
❓ Same here.
…set.
Minor refactoring of some collector code.
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
We want to move to the newest version of the opentelemetry-collector-contrib, however, there was a change with the
resourcedetectionprocessor where having zero detectors set results in zero "successes" which results in a failure, killing the collector:open-telemetry/opentelemetry-collector-contrib@50cd2e8
We therefore need to change our code so that we only add the
resourcedetectionprocessor if we have detectors set.What is the new behavior (if this is a feature change)?
We only add the
resourcedetectionprocessor to collector pipelines if there are detectors set in thepostgrescluster/pgadminmanifests. Allows us to move to otel v0.144.0.Other Information: