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: remove zero count metrics from registry #2163

Merged
merged 6 commits into from
Jul 21, 2021

Conversation

astorm
Copy link
Contributor

@astorm astorm commented Jul 20, 2021

Closes #1977

Checklist

  • Implement code
  • Add tests
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@astorm astorm marked this pull request as draft July 20, 2021 23:06
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jul 20, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Jul 20, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-07-21T17:13:37.259+0000

  • Duration: 18 min 56 sec

  • Commit: d708fa6

Test stats 🧪

Test Results
Failed 0
Passed 20
Skipped 0
Total 20

Trends 🧪

Image of Build Times

Image of Tests

@@ -144,7 +159,7 @@ test('does not include breakdown when not sampling', t => {

const { metricsets } = data

assertMetricSet(t, 'transaction', metricsets, {
assertMetricSet(t, 'transaction not sampling', metricsets, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this test asserted that transaction.breakdown.count was reported. However, with the new behavior the transaction.breakdown.count metric is no longer reported in this specific scenario because its count is zero.

@astorm
Copy link
Contributor Author

astorm commented Jul 21, 2021

jenkins run the tests please

@astorm astorm requested a review from trentm July 21, 2021 02:52
@astorm astorm marked this pull request as ready for review July 21, 2021 16:09
Copy link
Member

@trentm trentm left a comment

Choose a reason for hiding this comment

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

Just the changelog nit. Otherwise looks perfect.

Comment on lines 42 to 43
* The agent will no longer report couting metrics with a value of zero, and will
remove these metrics from the registry.
Copy link
Member

Choose a reason for hiding this comment

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

  • s/couting/counting/
  • Issue link please.

lib/metrics/reporter.js Show resolved Hide resolved
@astorm astorm merged commit aa86d4e into master Jul 21, 2021
@astorm astorm deleted the astorm/no-zero-count-metrics branch July 21, 2021 19:09
dgieselaar pushed a commit to dgieselaar/apm-agent-nodejs that referenced this pull request Sep 10, 2021
* feat: no longer sends counting metrics with a count of zero and removes them from the metrics cache/registry

Closes: elastic#1977
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agent-nodejs Make available for APM Agents project planning.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MetricsRegistry and High Cardinality
3 participants