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

Metrics updates #1054

Merged
merged 2 commits into from
Nov 21, 2022
Merged

Metrics updates #1054

merged 2 commits into from
Nov 21, 2022

Conversation

hananiel
Copy link
Contributor

@hananiel hananiel commented Nov 14, 2022

Description

Adds metrics for clr.cpu.count
Adds an AllowList Option to only add metrics needed, as opposed to listing all unneeded metrics
for example:

 "Metrics":{                                              
   "Observer":{                                 
     "EventCounterEvents": true,                
     "IncludedMetrics": ["cpu-usage"]           
     }                                            
  }                                              

Quality checklist

  • Your code complies with our Coding Style.
  • You've updated unit and/or integration tests for your change, where applicable.
  • You've updated documentation for your change, where applicable.
    If your change affects other repositories, such as Documentation, Samples and/or MainSite, add linked PRs here.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.
  • You've added required license files and/or file headers (explaining where the code came from with proper attribution), where code is copied from StackOverflow, a blog, or OSS.

@hananiel hananiel added the Component/Management Issues related to Steeltoe Management (actuators) label Nov 14, 2022
@hananiel hananiel self-assigned this Nov 14, 2022
@hananiel
Copy link
Contributor Author

/azp run Steeltoe.All

@hananiel hananiel requested a review from TimHess November 14, 2022 21:10
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@sonarcloud
Copy link

sonarcloud bot commented Nov 14, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@TimHess TimHess left a comment

Choose a reason for hiding this comment

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

looks OK to me, wouldn't be a bad idea to address the sonar issue now but isn't required in this branch

@bart-vmware
Copy link
Member

looks OK to me, wouldn't be a bad idea to address the sonar issue now but isn't required in this branch

Just because no warning is produced? In that case, I'm afraid I have to disagree. In that same reasoning, it would be okay to introduce race conditions. Writing proper English is something we should do regardless of tooling.

I don't expect this PR to fix all existing docs, but we should get it right from the start for new ones. There's no point in postponing. The effort is minimal and it needs to be done anyway. Postponing only makes it harder to merge to main at a later time.

@TimHess
Copy link
Member

TimHess commented Nov 18, 2022

looks OK to me, wouldn't be a bad idea to address the sonar issue now but isn't required in this branch

Just because no warning is produced? In that case, I'm afraid I have to disagree. In that same reasoning, it would be okay to introduce race conditions. Writing proper English is something we should do regardless of tooling.

I don't expect this PR to fix all existing docs, but we should get it right from the start for new ones. There's no point in postponing. The effort is minimal and it needs to be done anyway. Postponing only makes it harder to merge to main at a later time.

I get your point, and in that case my response would be different, but in this particular case we're talking about two periods that probably won't even be noticed by anybody. In the future, for trivial changes like this I think it would be nice to include the suggested change up front (when possible) for a quicker resolution

@hananiel hananiel merged commit 9e25228 into release/3.2 Nov 21, 2022
@hananiel hananiel deleted the MetricsUpdates branch November 21, 2022 17:27
@hananiel hananiel mentioned this pull request Nov 21, 2022
@TimHess TimHess added this to the 3.2.2 milestone Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component/Management Issues related to Steeltoe Management (actuators)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants