Try fix TestMetricsExport #2088
Conversation
knative-prow-robot
added
the
size/XS
| if newConfig.backendDestination == prometheus { | ||
| return newConfig.prometheusHost != cc.prometheusHost || newConfig.prometheusPort != cc.prometheusPort | ||
| } | ||
|
|
There was a problem hiding this comment.
I am wondering why we didn't have this here before? Probably it is because we dont allow so far to change the host and port, but it does not hurt anything here I guess since our observability cm does not expose it anyway.
There was a problem hiding this comment.
I wonder if all the effort to avoid reloading the config is too clever by half, but this seems like a good fix for now.
Codecov Report
@@ Coverage Diff @@
## main #2088 +/- ##
==========================================
- Coverage 67.36% 67.31% -0.06%
==========================================
Files 215 215
Lines 9111 9092 -19
==========================================
- Hits 6138 6120 -18
+ Misses 2698 2697 -1
Partials 275 275
Continue to review full report at Codecov.
|
|
@markusthoemmes pls review. |
|
@evankanderson pls review. |
|
/assign @evankanderson |
knative-prow-robot
added
lgtm
| if newConfig.backendDestination == prometheus { | ||
| return newConfig.prometheusHost != cc.prometheusHost || newConfig.prometheusPort != cc.prometheusPort | ||
| } | ||
|
|
There was a problem hiding this comment.
I wonder if all the effort to avoid reloading the config is too clever by half, but this seems like a good fix for now.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: evankanderson, n3wscott The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
failing test seems related. It's the test that's supposed to be fixed, I guess? |
|
The failing test is in the OpenCensus path, so it's not directly related to this PR. It looks like maybe we missed an export RPC in that test run. |
|
@evankanderson The failing path is due to the timeout where we dont get any records. Prometheus has a 10sec delay for the endpoint to be ready. Maybe if we are more generous to opencensus this will not happen (assuming there is no race there or some bug at the opencensus side).. |
|
/retest |
|
The last time time I fixed this, I discovered that we were using the first four reports, rather than waiting for four different series. I'm not sure what's happened in this case. |
TestMetricsExport creates a lot of failures on CI lately. Here is a case where exporter never got updated. This means most likely that a previous test already had the backend set as expected but the port was different than the one expected later on.
I guess this happens only in tests and #1672 is not fixed.
This fix makes logic about prometheus reconfiguration correct for testing purposes.
/cc @evankanderson