Update client_golang#42527
Conversation
| JobName: "job1", | ||
| JobName: "job1", | ||
| // XXXXXXXXXXXXXXXx bad | ||
| MetricRelabelRegex: relabel.MustNewRegexp("regex2"), |
There was a problem hiding this comment.
the test is returning regex1, indicating that the relabel config is not getting updated properly. I cannot figure out how this change would have done that. race condition?
There was a problem hiding this comment.
Yes I think this is a race condition. I think the test is being run before the second scrapeconfig call is made.
There was a problem hiding this comment.
The allocator waitgroup returns after only one response has been given, which is weirdly by design.
There was a problem hiding this comment.
The problem is that we call Refresh and then immediately check for the result, but the updated config takes a while to be set. So we need to find a way to wait until we know the update has been done.
| found := false | ||
| var lastGot string | ||
| // The manager may not be done processing the Refresh call by the | ||
| // time we check the value of the ScrapeConfig, so retry a couple | ||
| // times to see if it fixes itself. | ||
| RETRY: | ||
| for j := 0; j < 3; j++ { | ||
| for _, sc := range manager.promCfg.ScrapeConfigs { | ||
| if sc.JobName == s.MetricRelabelConfig.JobName { | ||
| for _, mc := range sc.MetricRelabelConfigs { | ||
| lastGot = mc.Regex.String() | ||
| if s.MetricRelabelConfig.MetricRelabelRegex.String() == mc.Regex.String() { | ||
| found = true | ||
| break RETRY | ||
| } | ||
| } | ||
| } | ||
| } | ||
| time.Sleep(2 * time.Second) | ||
| } | ||
| if !found { | ||
| t.Errorf("MetricRelabelConfig did not match expectation, want %v got %v", s.MetricRelabelConfig.MetricRelabelRegex.String(), lastGot) |
There was a problem hiding this comment.
I will heartily welcome a better way to do this. I don't think we can use a waitgroup because the thing we are waiting for is the sync() call in the manager, and there is no way I can see for a test to tie in to that logic to know when a sync has finished.
ywwg
requested review from
a team,
ArthurSens,
dashpole,
fatsheep9146,
mx-psi and
songy23
as code owners
github-actions
Bot
added
receiver/simpleprometheus
receiver/purefa
receiver/prometheus
github-actions
Bot
requested review from
Aneurysm9,
IbraheemA,
ankitpatel96,
chrroberts-pure,
dgoscn,
dineshg13,
jade-guiton-dd,
krajorama,
liustanley and
mackjmr
songy23
left a comment
songy23
left a comment
There was a problem hiding this comment.
the target branch open-telemetry:otelbot/update-otel-1757061301 is already stale, you may want to recreate the PR to mainline
|
|
||
| baseCfg := promconfig.Config{GlobalConfig: promconfig.DefaultGlobalConfig} | ||
| manager := NewManager(receivertest.NewNopSettings(metadata.Type), tc.cfg, &baseCfg, false) | ||
| baseCfg, err := promconfig.Load("", nil) |
There was a problem hiding this comment.
You're changing from "DefaultGlobalConfig" to an empty string... totally shooting in the dark here, could this change influence the flakiness?
There was a problem hiding this comment.
Load pre-initializes with DefaultConfig (which contains DefaultGlobalConfig) and then merges the string into it. I compared the old init with the new, and the only diffs are the added Runtime and OtlpConfig items that were unset in the previous code.
There was a problem hiding this comment.
|
marking as draft because this is a discussion piece, not intended for direct merging |
ywwg
force-pushed
the
owilliams/update-otel
branch
from
70a2623 to
af8b7c3
Compare
|
well I tried to rebase and it was a gigantic disaster |
ywwg
force-pushed
the
owilliams/update-otel
branch
from
af8b7c3 to
ab268ae
Compare
|
github automatically closed this and I can't reopen. new PR at: #42568 |
4 participants
Fixes unset TextParser issues