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

Add a _doc_count field for single-histogram metricsets #4647

Merged
merged 4 commits into from
Jan 27, 2021

Conversation

axw
Copy link
Member

@axw axw commented Jan 21, 2021

Motivation/summary

If a metricset is published with a single histogram, add a _doc_count field which is the sum of the histogram's counts. This will enable us to remove some value_count aggregations in the UI to determine the transaction count.

Checklist

How to test these changes

  1. Enable transaction metrics aggregation
  2. Send some transactions
  3. Check that the docs have _doc_count in them
  4. Perform aggregations over the docs (e.g. terms agg on transaction.name), and check that the doc_count value in each bucket matches the number of transaction events aggregated.

Related issues

Closes #4474

If a metricset is published with a single histogram, add
a _doc_count field which is the sum of the histogram's
counts.
@apmmachine
Copy link
Contributor

apmmachine commented Jan 21, 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #4647 updated

    • Start Time: 2021-01-27T08:09:38.729+0000
  • Duration: 42 min 36 sec

  • Commit: fb57be1

Test stats 🧪

Test Results
Failed 0
Passed 4610
Skipped 127
Total 4737

Steps errors 4

Expand to view the steps failures

Run Window tests
  • Took 11 min 47 sec . View more details on here
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=coverage-files.tgz -czf coverage-files.tgz coverage
Compress
  • Took 0 min 0 sec . View more details on here
  • Description: tar --exclude=system-tests-linux-files.tgz -czf system-tests-linux-files.tgz system-tests
Test Sync
  • Took 3 min 10 sec . View more details on here
  • Description: ./.ci/scripts/sync.sh

@axw axw marked this pull request as ready for review January 21, 2021 13:21
@axw axw requested a review from a team January 21, 2021 13:21
@codecov-io
Copy link

codecov-io commented Jan 21, 2021

Codecov Report

Merging #4647 (2e355ff) into master (b456993) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4647      +/-   ##
==========================================
- Coverage   76.10%   76.06%   -0.05%     
==========================================
  Files         161      161              
  Lines        9892     9896       +4     
==========================================
- Hits         7528     7527       -1     
- Misses       2364     2369       +5     
Impacted Files Coverage Δ
model/metricset.go 95.08% <100.00%> (+0.34%) ⬆️
kibana/connecting_client.go 61.40% <0.00%> (-8.78%) ⬇️

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

Code looks good and I tested the change - everything works as expected against ES master.

Since _doc_count support was recently introduced, I also tested behavior against 7.9 ES. Writing the docs works fine, but running aggregations doesn't take the values into account. Since this impacts histogram functionality, I assume that Kibana would need to check for the ES version/indices when aggregating?

Screenshot 2021-01-25 at 15 24 13

Screenshot 2021-01-25 at 15 24 21

@dgieselaar
Copy link
Member

dgieselaar commented Jan 25, 2021

@simitt Kibana is only expected to work against ES versions of the same minor/major, so it's not expected to support ES 7.9 when running Kibana 7.12.

As transaction histograms are experimental we don't need to worry about backwards compatibility either (AFAIK).

@simitt
Copy link
Contributor

simitt commented Jan 25, 2021

Right! Perfectly fine then.

@axw
Copy link
Member Author

axw commented Jan 26, 2021

Sorry about the force-push, forgot I was working on a non-draft PR. About to merge anyway.

@axw
Copy link
Member Author

axw commented Jan 27, 2021

jenkins run the tests please

1 similar comment
@axw
Copy link
Member Author

axw commented Jan 27, 2021

jenkins run the tests please

@axw axw merged commit 5ec8404 into elastic:master Jan 27, 2021
@axw axw deleted the histogram-doc-count branch January 27, 2021 08:53
axw added a commit to axw/apm-server that referenced this pull request Feb 18, 2021
* model: _doc_count for single histogram metricsets

If a metricset is published with a single histogram, add
a _doc_count field which is the sum of the histogram's
counts.

* systemtest: check _doc_count in txmetrics docs
# Conflicts:
#	changelogs/head.asciidoc
axw added a commit that referenced this pull request Feb 18, 2021
[7.x] Add a _doc_count field for single-histogram metricsets (#4647)
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.

Use _doc_count data type when indexing histograms
5 participants