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

Remove old metrics #126

Merged
merged 7 commits into from
Jun 12, 2024
Merged

Remove old metrics #126

merged 7 commits into from
Jun 12, 2024

Conversation

ushitora-anqou
Copy link
Contributor

This commit removes the following metrics:

  • pie_io_write_latency_seconds
    - use pie_io_write_latency_on_mount_probe_seconds instead
  • pie_io_read_latency_seconds
    - use pie_io_read_latency_on_mount_probe_seconds instead
  • pie_create_probe_total
    - use pie_mount_probe_total or pie_provision_probe_total instead
  • pie_performance_probe_total
    - use pie_performance_on_mount_probe_total instead

@ushitora-anqou ushitora-anqou force-pushed the remove-old-metrics branch 4 times, most recently from 1e0640e to db39a5b Compare June 6, 2024 09:32
@ushitora-anqou ushitora-anqou marked this pull request as ready for review June 6, 2024 09:37
@ushitora-anqou ushitora-anqou requested a review from a team as a code owner June 6, 2024 09:37
Copy link
Contributor

@llamerada-jp llamerada-jp left a comment

Choose a reason for hiding this comment

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

Could you please review constants.go? Some values are not used.

ProbeThreshold string `json:"probeThreshold"`
NodeSelector corev1.NodeSelector `json:"nodeSelector"`

//+kubebuilder:validation:Maximum:=59
Copy link
Contributor

Choose a reason for hiding this comment

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

If parameters are required, woud you please add +kubebuilder:validation:Required annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be preferable to set default values for ProbeThreshold and ProbePeriod, similar to how their command-line argument counterparts had default values. I fixed my code: 8aed789 36f60e1

cmd/controller.go Show resolved Hide resolved
docs/design.md Outdated Show resolved Hide resolved
@ushitora-anqou ushitora-anqou force-pushed the remove-old-metrics branch 2 times, most recently from f0c33be to 36f60e1 Compare June 11, 2024 08:20
@ushitora-anqou
Copy link
Contributor Author

@llamerada-jp @cupnes I updated my code, so could you review this PR again? The changes I made since you reviewed are as follows: https://github.com/topolvm/pie/compare/db39a5b5ab5592f4719d10b8a8843abd93567a7c..36f60e1#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
monitoringStorageClass: YOUR-STORAGE-CLASS-NAME # This field is mandatory.
# All other fields are optional.
nodeSelector:
nodeSelectorTerms:
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the indentation is too much.

Suggested change
nodeSelectorTerms:
nodeSelectorTerms:

# All other fields are optional.
nodeSelector:
nodeSelectorTerms:
- matchExpressions:
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Suggested change
- matchExpressions:
- matchExpressions:

nodeSelector:
nodeSelectorTerms:
- matchExpressions:
- key: foo
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

Suggested change
- key: foo
- key: foo

README.md Outdated
nodeSelectorTerms:
- matchExpressions:
- key: foo
operator: DoesNotExist
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto. (I think it would be good to remove four half-width spaces here.)

Suggested change
operator: DoesNotExist
operator: DoesNotExist

This commit removes the following metrics:

- pie_io_write_latency_seconds
        - use pie_io_write_latency_on_mount_probe_seconds instead
- pie_io_read_latency_seconds
        - use pie_io_read_latency_on_mount_probe_seconds instead
- pie_create_probe_total
        - use pie_mount_probe_total or pie_provision_probe_total instead
- pie_performance_probe_total
        - use pie_performance_on_mount_probe_total instead

Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
Signed-off-by: Ryotaro Banno <ryotaro.banno@gmail.com>
@cupnes cupnes merged commit d74f4ba into main Jun 12, 2024
7 checks passed
@cupnes cupnes deleted the remove-old-metrics branch June 12, 2024 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants