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

Template merge functionality #493

Closed
rahulguptajss opened this issue Sep 6, 2021 · 6 comments · Fixed by #505 or #555
Closed

Template merge functionality #493

rahulguptajss opened this issue Sep 6, 2021 · 6 comments · Fixed by #505 or #555
Labels

Comments

@rahulguptajss
Copy link
Contributor

For this discussion purpose default template is lun.yaml and custom template is custom_lun.yaml

Current behaviour of custom template creation is documented at https://github.com/NetApp/harvest/blob/main/cmd/collectors/zapiperf/README.md#creatingediting-subtemplates

This approach requires copying all contents from default and then modify the custom template. This only supports complete overwrite of default templates. There are problems with this approach.

  1. Any change in parameters to default templates from release to release will need manual diffing to maintain changes in custom template.
  2. This is not good user experience to copy all contents from default template to custom template.

Proposed Change in Functionality:
Proposed solution intend to only change when object key defined in default.yaml and custom.yaml are same.
Solution is to overwrite top level parameters defined in custom template. All other params should be taken from default template.

Definition of top level parameters:
name,query,object,counters,plugins,export_options are the top level elements in below example.

lun.yaml

name:                       Lun
query:                      lun-get-iter
object:                     lun

counters:
  lun-info:
    - ^node
    - ^path
    - ^qtree
    - size
    - size-used
    - ^state
    - ^^uuid
    - ^volume
    - ^vserver => svm

plugins:
  - LabelAgent:
    value_mapping: status state online `1`
    # metric label zapi_value rest_value `default_value`
    value_to_num: new_status state online online `0`
    # path is something like "/vol/vol_georg_fcp401/lun401"
    # we only want lun name, which is 4th element
    split: path `/` ,,,lun

export_options:
  instance_keys:
    - node
    - qtree
    - lun
    - volume
    - svm
  instance_labels:
    - state

custom_lun.yaml

plugins:
  - LabelAgent:
    # my custom status
    value_mapping: custom_status state online `0`

Post merge lun.yaml

name:                       Lun
query:                      lun-get-iter
object:                     lun

counters:
  lun-info:
    - ^node
    - ^path
    - ^qtree
    - size
    - size-used
    - ^state
    - ^^uuid
    - ^volume
    - ^vserver => svm

plugins:
  - LabelAgent:
    # my custom status
    value_mapping: custom_status state online `0`

export_options:
  instance_keys:
    - node
    - qtree
    - lun
    - volume
    - svm
  instance_labels:
    - state

From above example plugins (top level param) got overwritten.

Advantages of this approach:

  1. Better user experience.
  2. Easy to compare and overwrite default templates.
  3. Compare and merging all child elements inside top level elements may lead to confusion and manual comparision.
@rahulguptajss rahulguptajss added the feature New feature or request label Sep 6, 2021
@hashi825
Copy link

hashi825 commented Sep 8, 2021

If you aren't merging child elements of the dict keys then if you want to add one extra counter into your custom template you'd have to copy the whole contents of the counters dict again which is essentially back to the same issue of redefining the entire template, unless I'm reading it wrong?

For example above the post merge Lun.yaml loses important value mappings from the plugins dict being overwritten, if it's the default you'd want that merged with the custom template instead.

@rahulguptajss
Copy link
Contributor Author

Intention of not merging child element is to avoid confusion where one would have to think about final merged template. If we go with child merging as well, we can build a cli to show the final merged template for debugging purpose.

@rahulguptajss rahulguptajss self-assigned this Sep 8, 2021
@hashi825
Copy link

hashi825 commented Sep 9, 2021

It would be definitely easier without child merging to avoid confusion but need to figure out the templating use case where you want the majority of the default template but want to add an extra counter and export a new label for example, without merging that requires copying most of the existing template, which then creates issues if the default template gets upgraded and we are back to square one.

@cgrinds
Copy link
Collaborator

cgrinds commented Sep 9, 2021

thanks for clarifying @hashi825 - rahul and I discussed yesterday and agree that we need better support for the use-cases you mentioned above: adding counter, export new label, etc. He's working a PR that satisfies those cases as well as supporting the overwriting use-cases. stay tuned

rahulguptajss added a commit that referenced this issue Sep 15, 2021
rahulguptajss added a commit that referenced this issue Sep 15, 2021
@rahulguptajss rahulguptajss linked a pull request Sep 15, 2021 that will close this issue
@rahulguptajss
Copy link
Contributor Author

rahulguptajss commented Sep 15, 2021

  • Template Merge Feature
  • Add a cli to print merged template to console for debugging
  • update docs

rahulguptajss added a commit that referenced this issue Sep 15, 2021
rahulguptajss added a commit that referenced this issue Sep 16, 2021
cgrinds pushed a commit that referenced this issue Sep 23, 2021
cgrinds pushed a commit that referenced this issue Sep 23, 2021
cgrinds pushed a commit that referenced this issue Sep 23, 2021
cgrinds pushed a commit that referenced this issue Sep 23, 2021
@rahulguptajss rahulguptajss reopened this Sep 28, 2021
rahulguptajss added a commit that referenced this issue Oct 11, 2021
rahulguptajss added a commit that referenced this issue Oct 11, 2021
rahulguptajss added a commit that referenced this issue Oct 11, 2021
rahulguptajss added a commit that referenced this issue Oct 11, 2021
rahulguptajss added a commit that referenced this issue Oct 11, 2021
cgrinds pushed a commit that referenced this issue Oct 18, 2021
* feat: Template docs
Fixes #493
Co-authored-by: Chris Grindstaff <[email protected]>
@cgrinds cgrinds assigned cgrinds and unassigned rahulguptajss Nov 2, 2021
@cgrinds
Copy link
Collaborator

cgrinds commented Nov 2, 2021

Verified, commit: c37b608

@cgrinds cgrinds removed their assignment May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants