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

NMS-16953: Correct the WinRM Virtual Memory graph #7521

Merged
merged 4 commits into from
Dec 10, 2024

Conversation

dino2gnt
Copy link
Contributor

@dino2gnt dino2gnt commented Nov 7, 2024

Add corrected graph as a new file and suppress the incorrect graph to avoid creating an rpmnew on upgrades

See #7110 for previous version of this PR.

All Contributors

Contribution Checklist

  • Please make an issue in the OpenNMS issue tracker if there isn't one already.
    Once there is an issue, please:
    1. update the title of this PR to be in the format: ${JIRA-ISSUE-NUMBER}: subject of pull request
    2. update the Jira link at the bottom of this comment to refer to the real issue number
    3. prefix your commit messages with the issue number, if possible
    4. once you've created this PR, please link to it in a comment in the Jira issue
      Don't worry if this sounds like a lot, we can help you get things set up properly.
  • If this code is likely to affect the UI, did you name your branch with -smoke in it to trigger smoke tests?
  • If this is a new or updated feature, is there documentation for the new behavior?
  • If this is new code, are there unit and/or integration tests?
  • If this PR targets a foundation-* branch, does it try to avoid changing files in $OPENNMS_HOME/etc/?

What's Next?

A PR should be assigned at least 2 reviewers. If you know that someone would be a good person to review your code, feel free to add them.

If you need help making additions or changes to the documentation related to your changes, please let us know.

In any case, if anything is unclear or you want help getting your PR ready for merge, please don't hesitate to say something in the comments here,
or in the #opennms-development chat channel.

Once reviewer(s) accept the PR and the branch passes continuous integration, the PR is eligible for merge.

At that time, if you have commit access (are an OpenNMS Group employee or a member of the OGP) you are welcome to merge the PR when you're ready.
Otherwise, a reviewer can merge it for you.

Thanks for taking time to contribute!

External References

Copy link
Contributor

@marshallmassengill marshallmassengill left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@cgorantla cgorantla left a comment

Choose a reason for hiding this comment

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

This will render two graphs, one would be right one and other broken with the same name as Virtual Memory Usage (WinRM).

Is that desirable ?

@dino2gnt
Copy link
Contributor Author

dino2gnt commented Nov 9, 2024

This will render two graphs, one would be right one and other broken with the same name as Virtual Memory Usage (WinRM).

Is that desirable ?

It shouldn't. The suppress value should hide the incorrect graph if the correct graph can be rendered instead.

@indigo423
Copy link
Member

indigo423 commented Nov 10, 2024

This will render two graphs, one would be right one and other broken with the same name as Virtual Memory Usage (WinRM).
Is that desirable ?

It shouldn't. The suppress value should hide the incorrect graph if the correct graph can be rendered instead.

  1. Why do we want to maintain an incorrect graph?

  2. I would always vote for tracking JIRA issues with setting fix versions to generate correct changelogs. My previous job background running OpenNMS systems for customers gives me a strong empathy for people relying on changelogs. Having changes bypassing the changelog, decreases a) transparency and b) trustworthiness and usefulness of the changelog. Besides that, we need to maintain a lot of versions in different GitHub repos, which makes it extremely hard to track changes down when they are merged without JIRA issues.

@dino2gnt
Copy link
Contributor Author

  1. Why do we want to maintain an incorrect graph?

This method allows us to avoid configuration changes / rpmnew files on upgrade, while fixing the graph in all supported branches. It's just a memory graph, but it's wrong, and providing incorrect data to the user is a cardinal sin.

@dino2gnt dino2gnt changed the title Correct the WinRM Virtual Memory graph NMS-16953: Correct the WinRM Virtual Memory graph Nov 12, 2024
@dino2gnt dino2gnt requested a review from indigo423 November 12, 2024 16:44
@indigo423
Copy link
Member

indigo423 commented Nov 12, 2024

  1. Why do we want to maintain an incorrect graph?

This method allows us to avoid configuration changes / rpmnew files on upgrade, while fixing the graph in all supported branches. It's just a memory graph, but it's wrong, and providing incorrect data to the user is a cardinal sin.

Ok got it, what about removing the wrong graph in develop for H34, that we have a path forward to get rid of it :)

Copy link
Member

@indigo423 indigo423 left a comment

Choose a reason for hiding this comment

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

We need more freak bytes :)

…ies.d/wsman-microsoft-windows-virtmem.properties

Co-authored-by: Ronny Trommer <[email protected]>
@dino2gnt
Copy link
Contributor Author

Ok got it, what about removing the wrong graph in develop for H34, that we have a path forward to get rid of it :)

Sure, I can submit a PR for that.

@dino2gnt
Copy link
Contributor Author

dino2gnt commented Dec 4, 2024

what about removing the wrong graph in develop for H34, that we have a path forward to get rid of it :)

#7552

Copy link
Contributor

@cgorantla cgorantla left a comment

Choose a reason for hiding this comment

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

lgtm!

@dino2gnt dino2gnt merged commit 71e9426 into foundation-2021 Dec 10, 2024
8 checks passed
@dino2gnt dino2gnt deleted the dcy/FixWinVirtMemGraph branch December 10, 2024 19:51
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.

4 participants