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

[target-allocator] Fix bug found when using relabel-config filterStrategy with serviceMonitors #1232

Conversation

moh-osman3
Copy link
Contributor

@moh-osman3 moh-osman3 commented Nov 8, 2022

Resolves #1231

Also adds to go.mod

replace github.com/prometheus/prometheus => github.com/prometheus/prometheus v0.38.0

to use the latest version of prometheus. This should avoid other dependencies bringing in older versions of prometheus, which is missing a relabel method I'm using.

@moh-osman3 moh-osman3 force-pushed the mohosman/issue-1231/servicemonitors-relabel-filter branch from 5577956 to aa8201e Compare November 8, 2022 09:29
@moh-osman3 moh-osman3 marked this pull request as ready for review November 8, 2022 21:57
@moh-osman3 moh-osman3 requested a review from a team November 8, 2022 21:57
cmd/otel-allocator/prehook/relabel.go Outdated Show resolved Hide resolved
cmd/otel-allocator/prehook/relabel.go Outdated Show resolved Hide resolved
@moh-osman3 moh-osman3 force-pushed the mohosman/issue-1231/servicemonitors-relabel-filter branch from 2e57707 to 08a34e6 Compare November 10, 2022 04:45
Comment on lines 92 to 103
func (tf *RelabelConfigTargetFilter) replaceRelabelConfig(cfg []*relabel.Config) []*relabel.Config {
outCfg := make([]*relabel.Config, len(cfg))
out, err := yaml2.Marshal(cfg)
if err != nil {
tf.log.V(2).Info("Error Marshaling", "error", err)
return cfg
}

byteArr := replaceShard([]byte(out))
err = yaml2.Unmarshal(byteArr, &outCfg)
if err != nil {
tf.log.Info("Error Unmarshalling", "error", err)
return cfg
}

return outCfg
}

Copy link
Contributor

Choose a reason for hiding this comment

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

rather than marshaling and then unmarshaling, it's probably more efficient to just iterate through the relabel config list and do the replacement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Changed according to this suggestion.

// See this thread [https://github.com/open-telemetry/opentelemetry-operator/pull/1124/files#r983145795]
// for why SHARD == 0 is a necessary substitution. Otherwise the keep action that uses this env variable,
// would not match the regex and all targets end up dropped.
shard := "0"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: also should include that shard will always be 0 here, and it doesn't make sense to read the SHARD from the environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@moh-osman3 moh-osman3 force-pushed the mohosman/issue-1231/servicemonitors-relabel-filter branch from cdccb88 to e993c52 Compare November 16, 2022 18:29
@moh-osman3 moh-osman3 requested review from jaronoff97 and secustor and removed request for jaronoff97 November 16, 2022 19:03
@moh-osman3 moh-osman3 force-pushed the mohosman/issue-1231/servicemonitors-relabel-filter branch from e993c52 to 2554098 Compare November 18, 2022 18:51
@pavolloffay
Copy link
Member

@jaronoff97 can this be merged?

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

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

@pavolloffay once the conflicts are fixed, this should be good to go! (cc @moh-osman3)

@moh-osman3 moh-osman3 force-pushed the mohosman/issue-1231/servicemonitors-relabel-filter branch from 2554098 to 52cfabc Compare November 21, 2022 21:01
@moh-osman3
Copy link
Contributor Author

@pavolloffay once the conflicts are fixed, this should be good to go! (cc @moh-osman3)

Just finished resolving merge conflicts and retested in my cluster! Thanks for the review y'all!

@pavolloffay pavolloffay merged commit dc3c177 into open-telemetry:main Nov 22, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
…tegy with serviceMonitors (open-telemetry#1232)

* env variable substitution for

* remove unnessary env variable lookup and gofmt

* fix linter issues

* addressing review feedback

* add to unit test

* addressing feedback

* change go.mod to use later version of prometheus

* run gofmt

* rebasing go.mod and running go mod tidy
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] targets get dropped when using relabel-config Filter with serviceMonitor
5 participants