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

Replication observability metrics #142

Merged
merged 11 commits into from
Oct 8, 2024
Merged

Conversation

nitesh3108
Copy link
Contributor

@nitesh3108 nitesh3108 commented Oct 8, 2024

PR Submission checklist

It enables API support for volume_mirror_transfer_rate_cma_view which will be helpful to pull the Replication related stats for Sync and Metro at volume level.

GitHub Issues

List the GitHub issues impacted by this PR:

GitHub Issue #
dell/csm#1443

Common PR Checklist:

  • Have you made sure that the code compiles?
  • Have you commented your code, particularly in hard-to-understand areas
  • Did you run tests in a real Kubernetes cluster?
  • Have you maintained backward compatibility

Description of your changes:

  • Executed gopowerstore unit tests

Earlier unit-test report

PASS
coverage: 82.2% of statements
ok      github.com/dell/gopowerstore    0.069s  coverage: 82.2% of statements

Current unit-test report

=== RUN   TestClientIMPL_VolumeMirrorTransferRate
--- PASS: TestClientIMPL_VolumeMirrorTransferRate (0.00s)

PASS
coverage: 83.8% of statements
ok      github.com/dell/gopowerstore    0.064s  coverage: 83.8% of statements

metrics.go Show resolved Hide resolved
metrics.go Outdated
// VolumeMirrorTransferRate - Volume Mirror Transfer Rate
func (c *ClientIMPL) VolumeMirrorTransferRate(ctx context.Context, entityID string) ([]VolumeMirrorTransferRateResponse, error) {
var resp []VolumeMirrorTransferRateResponse
err := c.mirrorTransferRate(ctx, &resp, entityID, 2000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this 2000 be a constant value instead of hardcoding it here like this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nitesh3108 nitesh3108 force-pushed the replication-observability-metrics branch from 863ab1e to 04451e6 Compare October 8, 2024 12:38
adarsh-dell
adarsh-dell previously approved these changes Oct 8, 2024
Copy link
Contributor

@adarsh-dell adarsh-dell left a comment

Choose a reason for hiding this comment

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

LGTM.

if err != nil {
err = WrapErr(err)
}
customHeader.Del("DELL-VISIBILITY")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
customHeader.Del("DELL-VISIBILITY")
customHeader.Del("DELL-VISIBILITY")
apiClient.SetCustomHTTPHeaders(customHeader)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

adarsh-dell
adarsh-dell previously approved these changes Oct 8, 2024
@nitesh3108 nitesh3108 merged commit 6990d1b into main Oct 8, 2024
4 checks passed
@nitesh3108 nitesh3108 deleted the replication-observability-metrics branch October 8, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants