update registration of the metrics to work in techpreview too #808
Conversation
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
do-not-merge/work-in-progress
tremes
force-pushed
the
gather-job-metrics
branch
2 times, most recently
from
0859f8e
to
78396fc
Compare
tremes
force-pushed
the
gather-job-metrics
branch
from
733d302
to
52bddec
Compare
tremes
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
/test e2e-gcp-ovn-techpreview |
2 similar comments
|
/test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview |
| apierrors "k8s.io/apimachinery/pkg/api/errors" | ||
| ) | ||
|
|
||
| func (c *Client) SendAndGetID(ctx context.Context, endpoint string, source Source) (string, error) { | ||
| func (c *Client) SendAndGetID(ctx context.Context, endpoint string, source Source) (string, int, error) { |
There was a problem hiding this comment.
I'm having some concerns about confusing use of second return parameter. This is part of the same nit with upcoming comments. I prefer to leave the comments inline to avoid more confusion :D
As a summary, I'd assign a variable to default returning code, to improve readability. Then later use the same codes as parameters for a control function, moving the transformation of the data to that function itself. That way code is more straightforward IMHO, and avoids confusion on why transforming the data, why sending a 0 as a string, 0 int returnings with no context, etc...
I think it's a simple change but that improves readability and avoids any confusion on why or what the data is used for.
Please, check upcoming messages 👇
| cv, err := c.GetClusterVersion() | ||
| if apierrors.IsNotFound(err) { | ||
| return "", ErrWaitingForVersion | ||
| return "", 0, ErrWaitingForVersion |
There was a problem hiding this comment.
var or const 0 as defaultCode to guide about what the value will meant in advance
| counterRequestSend.WithLabelValues(c.metricsName, "0").Inc() | ||
| return "", fmt.Errorf("unable to build request to connect to Insights server: %v", err) | ||
| // if the request is not build, for example because of invalid endpoint,(maybe some problem with DNS), we want to have record about it in metrics as well. | ||
| insights.IncrementCounterRequestSend("0") |
There was a problem hiding this comment.
Send defaultCode (variable or const for default 0 value) as it is, the data transformation should take place inside IncrementCounterRequestSend function
There was a problem hiding this comment.
e.g. insights.IncrementCounterRequestSend(defaultCode)
I think it's more clear what the 0 means, without even reading the comment above.
| @@ -58,36 +59,36 @@ func (c *Client) SendAndGetID(ctx context.Context, endpoint string, source Sourc | |||
| } | |||
| }() | |||
|
|
|||
| counterRequestSend.WithLabelValues(c.metricsName, strconv.Itoa(resp.StatusCode)).Inc() | |||
| insights.IncrementCounterRequestSend(strconv.Itoa(resp.StatusCode)) | |||
There was a problem hiding this comment.
Send directly status code, same as before the transformation will take place inside IncrementCounterRequestSend
e.g. insights.IncrementCounterRequestSend(resp.StatusCode)
pkg/insights/metrics.go
Outdated
| MustRegisterMetrics(RecommendationCollector, counterRequestSend) | ||
| } | ||
|
|
||
| func IncrementCounterRequestSend(status string) { |
There was a problem hiding this comment.
change signature to accept int instead. Http codes type is int and we can play with a default value of the same type. Then before sending the data, transform it with strconv or fmt as string.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ncaak, tremes 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 |
|
/test e2e |
|
/test e2e-agnostic-upgrade |
|
/test e2e-gcp-ovn-techpreview |
|
@tremes: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
techpreview testsuite passed including the basic metrics test. I am adding the qe-approved label myself. |
openshift-ci
bot
added
the
qe-approved
…ift#808) * update registration of the metrics to work in techpreview too * updates after review * forgotten zeroes...
This PR updates the logic of Prometheus metrics registration to work in techpreview (when data gathering runs in a job) as well. The registration and incrementation of the
insightsclient_request_send_totalmetric is moved from the uploader and the metric in the techpreview is updated based on the conditions of the latestdatagatherCR (which provides information including HTTP status code in its conditions).This also removes the
insightsclient_last_gather_timemetric. This data is available in the Insights CRs.This also removes
Categories
Sample Archive
no new data
Documentation
Unit Tests
Privacy
Yes. There are no sensitive data in the newly collected information.
Changelog
Breaking Changes
No
References
https://issues.redhat.com/browse/???
https://access.redhat.com/solutions/???