Skip to content
This repository has been archived by the owner on Aug 28, 2024. It is now read-only.

Remote secret rejected data upload metric #179

Merged
merged 16 commits into from
Oct 25, 2023

Conversation

mshaposhnik
Copy link
Collaborator

@mshaposhnik mshaposhnik commented Oct 18, 2023

What does this PR do?

PR adds metrics for the failed upload data operations, with the operation name and failure reason labels, so the counterscan be filtered/ordered by them.

Screenshot/screencast of this PR

image

What issues does this PR fix or reference?

https://issues.redhat.com/browse/SVPI-575

How to test this PR?

  1. Need to setup minikube with moniroting (PR is on the way)
  2. Try to upload data with some error condidion (like duplicate field, or bad source secret etc)
    (can use the remote-secret-conflicting-targets sample or example below )
apiVersion: appstudio.redhat.com/v1beta1
kind: RemoteSecret
metadata:
  name: myrs1
  namespace: target-namespace
spec:
  secret:
    generateName: copied-
dataFrom:
  name: non-existent-secret
  namespace: default
  1. Verify counter incremented with correct labels

Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
@openshift-ci
Copy link

openshift-ci bot commented Oct 18, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@mshaposhnik mshaposhnik changed the title Spi rejected metric Spi rejected data upload metric Oct 18, 2023
Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
Signed-off-by: Max Shaposhnyk <[email protected]>
Copy link
Contributor

@metlos metlos left a comment

Choose a reason for hiding this comment

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

Generally looks good with the nitpick of maybe precreating the differently labeled counters and incrementing. Not sure if that's even worth the trouble though, because it means saving a couple of allocations only in the error cases of which there should be few.

pkg/webhook/remotesecret_mutator.go Outdated Show resolved Hide resolved
Signed-off-by: Max Shaposhnyk <[email protected]>
@mshaposhnik mshaposhnik changed the title Spi rejected data upload metric Remote secret rejected data upload metric Oct 23, 2023
@mshaposhnik mshaposhnik marked this pull request as ready for review October 23, 2023 08:27
@mshaposhnik
Copy link
Collaborator Author

/retest

1 similar comment
@mshaposhnik
Copy link
Collaborator Author

/retest

Copy link
Collaborator

@skabashnyuk skabashnyuk left a comment

Choose a reason for hiding this comment

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

The code is working for me.
Checked

  • redhat_appstudio_remotesecret_data_upload_rejected_total{operation="webhook_validate",reason="unique_targets_check_failed"}
  • redhat_appstudio_remotesecret_data_upload_rejected_total{operation="secret_data_upload",reason="invalid_upload_secret"}
Знімок екрана 2023-10-24 о 22 49 55

Documentation

Let's think about some documentation before the merge. For example docs/Monitoring.md

Mertric Description
redhat_appstudio_remotesecret_data_upload_rejected_total{operation="webhook_validate",reason="unique_targets_check_failed"}` This is ...

Signed-off-by: Max Shaposhnyk <[email protected]>
@openshift-ci
Copy link

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mshaposhnik, skabashnyuk

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:
  • OWNERS [mshaposhnik,skabashnyuk]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants