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

feat: compile out the metrics #5944

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

brianp
Copy link
Contributor

@brianp brianp commented Nov 10, 2023

Description

Do not merge until testing is complete

We made metric features in the applications a compiler option but core was still including it all by default.

I had wanted to do something more elegant like swap in a mock for the metrics system which would have hopefully required less sprinkles of changes, but the metrics are currently being used to return types from the metrics system and operate on them. This is still possible to phase out but would take more time due to the need to refactor all the existing metrics modules.

Motivation and Context

Metrics should be opt-in, and not on by default.

How Has This Been Tested?

Mobile team is testing it now.

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels Nov 10, 2023
Copy link

github-actions bot commented Nov 10, 2023

Test Results (Integration tests)

31 tests   31 ✔️  15m 7s ⏱️
11 suites    0 💤
  2 files      0

Results for commit 1fa74d9.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 10, 2023

Test Results (CI)

1 257 tests   1 257 ✔️  11m 22s ⏱️
     39 suites         0 💤
       1 files           0

Results for commit 1fa74d9.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label Nov 13, 2023
@SWvheerden SWvheerden merged commit fa2fb27 into tari-project:development Nov 13, 2023
14 checks passed
@brianp brianp deleted the build-ffis-sans-metrics branch November 13, 2023 08:29
SWvheerden pushed a commit that referenced this pull request Nov 14, 2023
Description
---
Does minor refactoring of metrics-only blocks to make the analyzer
happy.

Motivation and Context
---
Recent work in #5944 makes metrics a non-default feature. This made the
analyzer unhappy due to some unused variables, but it turns out this can
be fixed while simplifying some of the handling of metrics-only blocks.
This PR makes these changes and keeps the analyzer happy.

How Has This Been Tested?
---
Existing tests pass.

What process can a PR reviewer use to test or verify this change?
---
Check that the new metrics-only block handling is identical to that
introduced by #5944.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants