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

feat: support flexgroup constituents in template #2410

Merged
merged 11 commits into from
Oct 17, 2023
Merged

Conversation

Hardikl
Copy link
Contributor

@Hardikl Hardikl commented Oct 6, 2023

Supporting one flag includeConstituents in all volume templates, which can decide to export flexgroup constituents or not from Harvest poller.

Changes:
For Zapi,
Config api gives all the records, flag includeConstituents will control constituents records in plugin.

For ZapiPerf,
Perf api gives all the records, flag includeConstituents will control constituents records in plugin.

For RestPerf,
Perf api gives all the records, flag includeConstituents will control constituents records in plugin.

For Rest,
Public api either gives Flexgroup and Flexvols Or only Flexgroup_constituents but not all.
Used private api gives all the records, Then call public api 2 times for fetching all config details for them. 1 with filter is_constituent = false and 2 with filter is_constituent = true. Later, flag includeConstituents will control constituents records in plugin.

One alternate option would be to duplicate volume template based on is_constituent filter, But I would not be inclined towards it as maintenance cost and complexity to User while update/add metrics in both.

@cla-bot cla-bot bot added the cla-signed label Oct 6, 2023
@Hardikl Hardikl marked this pull request as draft October 6, 2023 15:22
@Hardikl Hardikl linked an issue Oct 6, 2023 that may be closed by this pull request
@Hardikl Hardikl marked this pull request as ready for review October 11, 2023 11:33
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/zapi/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/zapi/plugins/volume/volume.go Outdated Show resolved Hide resolved
conf/rest/9.9.0/volume.yaml Outdated Show resolved Hide resolved
grafana/dashboards/cmode/volume.json Show resolved Hide resolved
@cgrinds
Copy link
Collaborator

cgrinds commented Oct 11, 2023

Any impact to published metrics?

cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
cmd/collectors/rest/plugins/volume/volume.go Outdated Show resolved Hide resolved
@Hardikl
Copy link
Contributor Author

Hardikl commented Oct 12, 2023

Any impact to published metrics?

This change would only increase the # records of the existing metrics, doesn't add/change the shape of existing ones.
No impact for published metrics.

rahulguptajss
rahulguptajss previously approved these changes Oct 13, 2023
Copy link
Contributor

@rahulguptajss rahulguptajss left a comment

Choose a reason for hiding this comment

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

looks good. Needs relevant changes if counters needs to be disabled by default.

@Hardikl
Copy link
Contributor Author

Hardikl commented Oct 16, 2023

looks good. Needs relevant changes if counters needs to be disabled by default.

changed to false for default.

@@ -79,7 +79,7 @@ plugins:
- Volume:
schedule:
- data: 15m # should be multiple of poll duration
include_constituents: true
include_constituents: false
Copy link
Contributor

Choose a reason for hiding this comment

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

needs dashboard text also about the steps needed to enable dashboard

@Hardikl
Copy link
Contributor Author

Hardikl commented Oct 16, 2023

How about this description ?
image

@cgrinds cgrinds merged commit bcd09c6 into main Oct 17, 2023
10 checks passed
@cgrinds cgrinds deleted the hl_flexgroup_support branch October 17, 2023 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Harvest should collect and display Flexgroup Informations
3 participants