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 rolling statistics to summary graph #351

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benghaem
Copy link

@benghaem benghaem commented Feb 9, 2022

Summary:
This commit adds the option to show rolling statistics as additional lines on the summary graph. Currently this can either be a rolling mean or percentile. The other aggregate statistics are preserved separately. This works for both tagged and untagged summary graphs. The feature is disabled by default.

Configuration is done with two new reporter_options like so:

summary("phoenix.router_dispatch.stop.duration",
        tags: [:route],
        unit: {:native, :millisecond},
        reporter_options: [
          derive_modes: ["p90", "mean"],
          derive_window_secs: 180
        ]
      )

The "mode" is either "mean" for mean, or "pX", where X is an integer from 0-100, for a percentile. The statistic is computed over a backwards looking window that contains the last N seconds as set by derive_window_secs.

On the javascript side we register new series and dataset with findOrCreateSeries() and modify the dataset object to include information on which statistics to compute and which series the new dataset is derived from.

Incidentally, I think this addresses #245.

Feedback appreciated! Thanks!

Test Plan:
metrics_live_test.js: I added new unit tests to cover the derived series cases and updated the old tests to account for the new dataset object.
chart_component_tests.exs: New unit tests similar to the ones for the pruning option.
dev.exs: I put an example usage in dev.exs and tested end-to-end manually

Example:
image

@josevalim
Copy link
Member

Hi @benghaem! This looks awesome, thank you for working on it. Here are some suggestions:

  1. I think we can automatically compute and include the mean in summaries
  2. Then what we need is only an option for percentiles and I think we can include [95] as the default

WDYT?

@benghaem
Copy link
Author

benghaem commented Feb 9, 2022

@josevalim

Sounds good to me! I'll make those changes.

@josevalim
Copy link
Member

@benghaem ping :)

@benghaem
Copy link
Author

Hey @josevalim. It's been a bit :) I'll roll those changes in this weekend.

@sofakingworld
Copy link

@benghaem Hi! This is good PR, could you please finish your work?

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.

3 participants