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

Fix parameter encoding issue #930

Merged
merged 8 commits into from
Jun 23, 2022

Conversation

jaronoff97
Copy link
Contributor

@jaronoff97 jaronoff97 commented Jun 14, 2022

The target allocator was only unencoding when a collector_id wasn't provided which made it so that the target for the collector was never returned. This also fixes the issue where the encoded URL wasn't provided correctly to the collectors

Closes #925

Evidence this works:

[ root@curl-jea:/ ]$ curl http://lightstep-collector-targetallocator:80/jobs/serviceMonitor%2Fcollector%2Fcoll
ector%2F0/targets
{"lightstep-collector-collector-2":{"_link":"/jobs/serviceMonitor%2Fcollector%2Fcollector%2F0/targets?collector_id=lightstep-collector-collector-2","targets":[{"targets":["0.0.0.0:8888"],"labels":null}]}}
          scrape_configs:
          - job_name: serviceMonitor/collector/collector/0
            honor_timestamps: true
            scrape_interval: 5s
            scrape_timeout: 5s
            metrics_path: /metrics
            scheme: http
            follow_redirects: true
            http_sd_configs:
            - follow_redirects: false
              url: http://lightstep-collector-targetallocator:80/jobs/serviceMonitor%2Fcollector%2Fcollector%2F0/targets?collector_id=$POD_NAME

@jaronoff97 jaronoff97 marked this pull request as ready for review June 15, 2022 17:59
@jaronoff97 jaronoff97 requested a review from a team June 15, 2022 17:59
Copy link
Member

@secustor secustor left a comment

Choose a reason for hiding this comment

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

A small nitpick, else lgtm.

Copy link
Contributor

@yuriolisa yuriolisa left a comment

Choose a reason for hiding this comment

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

LGTM.

@yuriolisa
Copy link
Contributor

@pavolloffay, could you please merge it?

@pavolloffay pavolloffay changed the title [925] Fix parameter encoding issue Fix parameter encoding issue Jun 23, 2022
@@ -2,17 +2,18 @@ package allocation

import (
"math"
logf "sigs.k8s.io/controller-runtime/pkg/log"
Copy link
Member

Choose a reason for hiding this comment

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

this should be moved to the second group

@pavolloffay pavolloffay merged commit ddeac3d into open-telemetry:main Jun 23, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* logging

* Fixed encoding issue of params

* Do string replacement

* Another test

* Removed dockerfile changes

* Fix broken test

* Newline

* Change placement of the log import
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.

Target Allocator not working as expected with ServiceMonitor CRDs
5 participants