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

fix(inputs.cisco_telemetry_mdt): Handle NXOS DME subtree telemetry format #15923

Merged
merged 6 commits into from
Oct 2, 2024

Conversation

yusufshalaby
Copy link
Contributor

@yusufshalaby yusufshalaby commented Sep 21, 2024

Summary

Fixes the alignment of dn tags with their corresponding fields for telemetry paths on NXOS where query-target=subtree.

Checklist

  • No AI generated code was used in this PR

Related issues

resolves #15922

@telegraf-tiger telegraf-tiger bot added fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels Sep 21, 2024
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks @yusufshalaby for your fix! I'm not sure I understand why your code-change should fix the issue? In principle your only actual modification is to delete the dn tag or am I getting this wrong?

@srebhan srebhan self-assigned this Sep 30, 2024
@yusufshalaby
Copy link
Contributor Author

yusufshalaby commented Sep 30, 2024

Hi @srebhan, thanks for reviewing this fix!

The main modification is that I'm setting the dn value of the tags map of each DME path group prior to adding the fields to the SeriesGrouper. The plugin currently mutates tags with a new dn and then carries that dn to the next path group, which is why I'm also deleting dn from tags after the loop. The new unit test should help clarify, but here's an example in json to make it easier to read. Suppose this is the input:

[
  {
    "descr": "this is eth1/1",
    "dn": "sys/intf/phys-[eth1/1]",
    "id": "eth1/1"
  },
  {
    "descr": "this is eth1/2",
    "dn": "sys/intf/phys-[eth1/2]",
    "id": "eth1/2"
  },
  {
    "descr": "this is eth1/3",
    "dn": "sys/intf/phys-[eth1/3]",
    "id": "eth1/3"
  }
]

Currently the plugin would parse this input as:

sys/intf descr="this is eth1/1"
sys/intf,dn="sys/intf/phys-eth[1/1]" id="eth1/1",descr="this is eth1/2"
sys/intf,dn="sys/intf/phys-eth[1/2]" id="eth1/2",descr="this is eth1/3"
sys/intf,dn="sys/intf/phys-eth[1/3]" id="eth1/3"

My simple fix is to first loop through the fields to find dn and update tags, then loop through the fields once again and add each field to the SeriesGrouper, ensuring they're all grouped together with the same tags:

sys/intf,dn="sys/intf/phys-eth[1/1]" descr="this is eth1/1",id="eth1/1"
sys/intf,dn="sys/intf/phys-eth[1/2]" descr="this is eth1/2",id="eth1/2"
sys/intf,dn="sys/intf/phys-eth[1/3]" descr="this is eth1/3",id="eth1/3"

@srebhan
Copy link
Member

srebhan commented Oct 2, 2024

Yeah but I don't see where the dn tag is used between setting it in line 629 and deleting it in line 637! That's why I'm asking. :-) Could you please point out where this is used?!?!

If the dn tag is used transiently, which is the case as you set and delete it unconditionally, it should be passed as a parameter instead of using tags for passing function params.

@yusufshalaby
Copy link
Contributor Author

yusufshalaby commented Oct 2, 2024

tags gets passed to parseContentField method on line 635 which passes tags (containing dn) to the grouper.add() on line 679. Is that what you're missing?

@srebhan
Copy link
Member

srebhan commented Oct 2, 2024

Yes that's what is missing. And you need to delete the tag to avoid it being carried over to the next node which might not define a dn at all. Is this correct? If so, please add a comment to the code stating the above!

@yusufshalaby
Copy link
Contributor Author

Yes, exactly. Added comments.

@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Oct 2, 2024

Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

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

Thanks for your effort and patience @yusufshalaby!

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Oct 2, 2024
@srebhan srebhan assigned DStrand1 and unassigned srebhan Oct 2, 2024
@DStrand1 DStrand1 merged commit 2268175 into influxdata:master Oct 2, 2024
26 of 27 checks passed
@github-actions github-actions bot added this to the v1.32.1 milestone Oct 2, 2024
srebhan pushed a commit that referenced this pull request Oct 7, 2024
asaharn pushed a commit to asaharn/telegraf that referenced this pull request Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cisco fix pr to fix corresponding bug plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cisco_telemetry_mdt input plugin does not properly handle subtree format for NXOS DME telemetry
3 participants