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

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

Merged
merged 15 commits into from
Sep 23, 2021
Merged

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

merged 15 commits into from
Sep 23, 2021

Conversation

tsullivan
Copy link
Member

@tsullivan tsullivan commented Sep 14, 2021 •
edited
Loading

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)

tsullivan
tsullivan commented Sep 14, 2021
View reviewed changes
x-pack/plugins/reporting/server/usage/get_reporting_usage.ts
@@ -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
jloleysens
jloleysens approved these changes Sep 14, 2021
View reviewed changes
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.

x-pack/plugins/reporting/server/usage/get_reporting_usage.ts Outdated
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 •
edited
Loading

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!

rudolf
rudolf approved these changes Sep 14, 2021
View reviewed changes
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

Bamieh
Bamieh approved these changes Sep 14, 2021
View reviewed changes
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 tsullivan mentioned this pull request Sep 23, 2021
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 23, 2021
@tsullivan
[Reporting] Add output size stats to telemetry metrics (elastic#112037)
aff884b 
* [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
@tsullivan
[Reporting] Add output size stats to telemetry metrics (#112037) (#11…
0621231 
…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
lykkin pushed a commit to lykkin/kibana that referenced this pull request Sep 28, 2021
@tsullivan @lykkin
[Reporting] Add output size stats to telemetry metrics (elastic#112037)
5afeb7e 
* [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
Reviewers

@rudolf rudolf rudolf approved these changes

@Bamieh Bamieh Bamieh approved these changes

@jloleysens jloleysens jloleysens approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tsullivan @elasticmachine @kibanamachine @rudolf @Bamieh @jloleysens @sophiec20