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.

Sign up for GitHub

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

Jump to bottom

Exclude gc_count from system-metrics instrumentation #1836

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Exclude gc_count from system-metrics instrumentation #1836

wants to merge 6 commits into from

Conversation

rahulhacker
Copy link
Contributor

@rahulhacker rahulhacker commented Jun 6, 2023 •
edited
Loading

Co-authored-by: SuryanarayanaPeri [email protected]

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1033

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@rahulhacker rahulhacker requested a review from a team June 6, 2023 12:33
@ocelotl
Copy link
Contributor

ocelotl commented Jun 13, 2023

Please add a more descriptive name to this PR and also fix lint.

@rahulhacker rahulhacker requested a review from ocelotl July 23, 2023 17:37
@rahulhacker
Copy link
Contributor Author

Please add a more descriptive name to this PR and also fix lint.

Lint issue is fixed

@ocelotl ocelotl changed the title Support for PyPy3 Add support for PyPy3 for instrumentation system metrics Aug 7, 2023
ocelotl
ocelotl suggested changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@ocelotl ocelotl 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 tests for these changes.

@rahulhacker
Copy link
Contributor Author

We need tests for these changes.

@ocelotl : Added the test cases

@rahulhacker
Copy link
Contributor Author

We need tests for these changes.

@ocelotl : Added the test cases

@ocelotl : Some of the checks are failing. Can you please help if missing anything, need some guidance??
As per the understanding we have added the code and test cases accordingly.

@SuryanarayanaPeri
Copy link
Contributor

@ocelotl - The work in this issue is to make the instrumentation either produce all the same metrics or exclude the instruments that don't have equivalents. Ideally at the end of this issue, the tests for this instrumentation library will be enabled in tox for pypy3.

For this we have excluded the instruments that don't have equivalents for the gc_count on pypy3. The test for the system metrics instrumentation is enabled in tox. Request you please review and merge the PR.

We will open a new feature request - to make the instrumentation to provide the gc_count metrics for pypy3.

@rahulhacker
Copy link
Contributor Author

We need tests for these changes.

@ocelotl :Added the test cases to support the pypy3 for system metrics instrumentation.

@ocelotl ocelotl changed the title Add support for PyPy3 for instrumentation system metrics Exclude gc_count from system-metrics instrumentation Oct 23, 2023
@ocelotl
Copy link
Contributor

ocelotl commented Oct 23, 2023

Renamed this PR to better reflect the changes being made here.

@ocelotl
Copy link
Contributor

ocelotl commented Oct 23, 2023

Ok, almost ready to approve and merge, just take a look at the comment above and implement the requested changes ✌️

@rahulhacker
Copy link
Contributor Author

@ocelotl : Please don't approve the PR, just hold for one or two days, will let you KNOW and then you can merge the PR

@rahulhacker
Copy link
Contributor Author

@ocelotl : Please don't approve the PR, just hold for one or two days, will let you KNOW and then you can merge the PR, reverting the logger changes

@rahulhacker rahulhacker marked this pull request as draft October 25, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ocelotl ocelotl Awaiting requested review from ocelotl

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[instrumentation-system-metrics] add support for pypy3
3 participants
@rahulhacker @ocelotl @SuryanarayanaPeri