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

Collector Instances Not Discovered Due to Case Sensitivity in matchLabels #3350

Open
nicolastakashi opened this issue Oct 12, 2024 · 7 comments · May be fixed by #3418
Open

Collector Instances Not Discovered Due to Case Sensitivity in matchLabels #3350

nicolastakashi opened this issue Oct 12, 2024 · 7 comments · May be fixed by #3418
Assignees
@davidhaja
Labels
bug Something isn't working needs triage

Comments

@nicolastakashi
Copy link
Contributor

nicolastakashi commented Oct 12, 2024 •
edited
Loading

Component(s)

target allocator

What happened?

Description

While running TA without using the OTel Operator, I spent some time trying to understand why the TA was not discovering my collector instances and just figured out that I was using this config file:

allocation_strategy: consistent-hashing
filter_strategy: relabel-config
collector_selector:
  matchLabels:
    app.kubernetes.io/instance: otel-integration
    app.kubernetes.io/name: opentelemetry-collector
prometheus_cr:
  enabled: true
  scrape_interval: 30s
  pod_monitor_selector: {}
  service_monitor_selector: {}

Highlight the matchLabels key. This is using camel case, which is the convention while working with Kubernetes resources, but TA’s unmarshal method is only accepting lower case.

Even though TA is using theLabelSelector under the k8s.io/apimachinery/pkg/apis/meta/v1 package, this is not working because this package doesn’t contain any annotations to YAML marshal.

Initially, I was thinking about converting the whole config into JSON and using the YAMLToJSON method available in the github.com/ghodss/yaml package, but after doing that I noticed the configs related to Prometheus from the github.com/prometheus/prometheus/config package stopped working, since they don’t support any JSON annotations for marshalling.

I had another idea in mind! Keep using YAML for marshalling and redefine theLabelMatcher struct inside the TA project.

I’d like to get your opinion on that and check if you folks see any other solution. Maybe ask Prometheus maintainers to add JSON annotations to the Config struct? Not sure. Looking forward to your feedback.

Steps to Reproduce

Start the TA using the following config

allocation_strategy: consistent-hashing
filter_strategy: relabel-config
collector_selector:
  matchLabels:
    app.kubernetes.io/instance: otel-integration
    app.kubernetes.io/name: opentelemetry-collector
prometheus_cr:
  enabled: true
  scrape_interval: 30s
  pod_monitor_selector: {}
  service_monitor_selector: {}

Expected Result

Be compatible with K8S semantics

Actual Result

Not compatibility with K8S semantics

Kubernetes Version

NA

Operator version

NA

Collector version

NA

Environment information

Environment

NA

Log output

No response

Additional context

No response
@nicolastakashi nicolastakashi added bug Something isn't working needs triage labels Oct 12, 2024
@davidhaja
Copy link
Contributor

I would like to work on this

@swiatekm
Copy link
Contributor

swiatekm commented Oct 23, 2024 •
edited
Loading

It would be great if we could figure out a way to do this without redefining all the K8s and Prometheus structs ourselves. I originally did it this way because it was simple and standalone TA wasn't a very popular thing to do. @davidhaja feel free to take a stab at it.

@nicolastakashi
Copy link
Contributor Author

@swiatekm Maybe if we implement a custon unmarshal function this could work, wdyt?

@swiatekm
Copy link
Contributor

Maybe, I'm honestly not that familiar with the unmarshalling magic for these structs. I'm hoping that someone more knowledgeable can make it work, or at least try different approaches to see what conclusively doesn't work.

@davidhaja davidhaja linked a pull request Nov 3, 2024 that will close this issue
Open
@davidhaja
Copy link
Contributor

The way I see is either we try to support the flexible/case insensitive unmarshalling in TA, or we change both the marshalling and unmarshalling to follow (and require) the k8s camel case convention.
The latter one is a breaking change, which I believe requires more effort. (Although probably results in cleaner logic, code as well)
In the submitted PR I've made some changes that implement the former option.

I've tried multiple yaml packages, but none of them supports case insensitive parsing of YAML files.

I ended up with github.com/goccy/go-yaml because of its feature set. I've described the feature that my change is using for providing case insensitive parsing.

@swiatekm swiatekm mentioned this issue Nov 5, 2024
Open
@MB175
Copy link

MB175 commented Nov 27, 2024

so in the mean time how do I get my target allocator to detect my collectors ?

@swiatekm
Copy link
Contributor

so in the mean time how do I get my target allocator to detect my collectors ?

Use the lowercase version: matchlabels. Note that this only matters if you're running the target allocator without the operator. The operator generates the right configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@davidhaja davidhaja

Labels
bug Something isn't working needs triage
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Support camelcase matchLabels and matchExpressions in TA config davidhaja/opentelemetry-operator
4 participants
@nicolastakashi @davidhaja @MB175 @swiatekm