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

[Reporting] Add output size stats to telemetry metrics #112037

Merged

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Sep 14, 2021

Summary

This PR adds size statistics on the reports in the cluster.

@tsullivan tsullivan added v8.0.0 Team:AppServices release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Sep 14, 2021
@tsullivan tsullivan requested review from a team as code owners September 14, 2021 00:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-reporting-services (Team:Reporting Services)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

@@ -161,7 +175,7 @@ export async function getReportingUsage(
jobTypes: {
terms: { field: JOB_TYPES_FIELD, size: DEFAULT_TERMS_SIZE },
aggs: {
appNames: { terms: { field: OBJECT_TYPES_FIELD, size: DEFAULT_TERMS_SIZE } }, // NOTE Discover/CSV export is missing the 'meta.objectType' field, so Discover/CSV results are missing for this agg
Copy link
Member Author

Choose a reason for hiding this comment

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

This comment is no longer valid as meta.objectType is added for all export types in the RequestHandler class.

@tsullivan tsullivan requested review from a team as code owners September 14, 2021 03:51
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

I did not test locally, but these changes look good to me 🍻

I have one open question that I'd like to get your thoughts on, happy to re-review if you agree with the proposal but I won't block on it.

Comment on lines 79 to 83
output_size: {
max: sizeMax?.value ?? null,
min: sizeMin?.value ?? null,
avg: sizeAvg?.value ?? null,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether a percentile based aggregation would not provide better insight into the questions we may have regarding output size (es docs).

Would it be more useful to know that the average output size is, e.g., 500kb or that 99% of outputs are below 700kb?

Percentile based aggregation can be more expensive to calcuate, which might disqualify it, but from what I can tell it looks pretty safe (also considering we are working with report documents so not sure there will be millions).

Something like:

output_size: {
  25th: ...,
  50th: ...,
  75th: ...,
  95th: ...,
  99th: ..., // outliers
}

Let me know what you think @tsullivan !

Copy link
Member Author

@tsullivan tsullivan Sep 14, 2021

Choose a reason for hiding this comment

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

++ I am in favor of this. At the time I implemented it with avg, I was hoping to use median instead, but there isn't an actual median aggregation. With percentiles, we get the median from the 50th percentile. This is good!

Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Core changes lgtm

Copy link
Member

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

schema changes LGTM

@tsullivan
Copy link
Member Author

@elasticmachine merge upstream

@tsullivan tsullivan force-pushed the reporting/telemetry-add-reports-stats branch from 3895259 to 9d6b579 Compare September 22, 2021 21:08
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@tsullivan tsullivan merged commit 03007d0 into elastic:master Sep 23, 2021
@tsullivan tsullivan deleted the reporting/telemetry-add-reports-stats branch September 23, 2021 23:14
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 23, 2021
* [Reporting] Add output size stats to telemetry metrics

* fix types

* add output_size for each jobtype

* add size metrics for each job type

* use more mock data in unit tests

* clean up test

* update test snapshots

* update telemetry mapping

* SizeMetrics => SizePercentiles

* DocCount interface

* fix tests

* Update get_export_stats.ts

* update snapshots
tsullivan added a commit that referenced this pull request Sep 24, 2021
…3057)

* [Reporting] Add output size stats to telemetry metrics

* fix types

* add output_size for each jobtype

* add size metrics for each job type

* use more mock data in unit tests

* clean up test

* update test snapshots

* update telemetry mapping

* SizeMetrics => SizePercentiles

* DocCount interface

* fix tests

* Update get_export_stats.ts

* update snapshots
@sophiec20 sophiec20 added the (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead label Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants