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

Telemetry features + docs #342

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

maelle
Copy link
Collaborator

@maelle maelle commented Nov 15, 2024

Fix #216
Fix #338

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 22490df is merged into main:

  • ✔️001_tpch_01: 23.9ms -> 24.1ms [-3.61%, +4.68%]
  • 🚀001_tpch_02: 126ms -> 64.2ms [-49.88%, -48.03%]
  • 🚀001_tpch_03: 66.6ms -> 41.2ms [-39.07%, -37.23%]
  • ✔️001_tpch_04: 23.9ms -> 23.7ms [-1.76%, +0.2%]
  • 🚀001_tpch_05: 120ms -> 64ms [-47.32%, -46.01%]
  • ✔️001_tpch_06: 15.1ms -> 15.2ms [-1.99%, +3.25%]
  • 🚀001_tpch_07: 139ms -> 78.1ms [-44.42%, -43.07%]
  • 🚀001_tpch_08: 158ms -> 101ms [-36.95%, -35.81%]
  • 🚀001_tpch_09: 130ms -> 68.8ms [-48.23%, -45.66%]
  • 🚀001_tpch_10: 76ms -> 50.5ms [-34.31%, -32.71%]
  • 🚀001_tpch_11: 64.2ms -> 39ms [-41.19%, -37.3%]
  • ✔️001_tpch_12: 29ms -> 28.8ms [-2.55%, +1.67%]
  • ✔️001_tpch_13: 20.1ms -> 20ms [-2.83%, +1.2%]
  • ❗🐌001_tpch_14: 21.2ms -> 21.6ms [+0.45%, +3.64%]
  • 🚀001_tpch_15: 63.7ms -> 37.6ms [-42.19%, -39.75%]
  • 🚀001_tpch_16: 66.3ms -> 41.3ms [-39.36%, -36.22%]
  • ✔️001_tpch_17: 27.5ms -> 27.4ms [-1.62%, +0.66%]
  • ✔️001_tpch_18: 23.5ms -> 23.6ms [-3.15%, +4.09%]
  • 🚀001_tpch_19: 129ms -> 68.7ms [-47.88%, -45.96%]
  • 🚀001_tpch_20: 78.7ms -> 52.4ms [-34.8%, -31.92%]
  • 🚀001_tpch_21: 173ms -> 82.5ms [-53.16%, -51.19%]
  • 🚀001_tpch_22: 126ms -> 68.7ms [-46.38%, -44.39%]
  • ✔️010_tpch_01: 84.6ms -> 86.8ms [-5.3%, +10.64%]
  • ✔️010_tpch_02: 72.8ms -> 71.3ms [-5.46%, +1.25%]
  • ✔️010_tpch_03: 65.9ms -> 66.2ms [-4.81%, +5.43%]
  • ✔️010_tpch_04: 49.3ms -> 50.7ms [-1.75%, +7.21%]
  • ✔️010_tpch_05: 98.1ms -> 95.2ms [-6.32%, +0.33%]
  • ✔️010_tpch_06: 37.4ms -> 37.2ms [-7.71%, +6.44%]
  • ✔️010_tpch_07: 117ms -> 119ms [-1.38%, +4.44%]
  • ✔️010_tpch_08: 136ms -> 136ms [-1.89%, +1.93%]
  • 🚀010_tpch_09: 120ms -> 118ms [-2.95%, -0.69%]
  • ✔️010_tpch_10: 84.6ms -> 83.3ms [-3.76%, +0.66%]
  • ✔️010_tpch_11: 39.7ms -> 38.3ms [-8.86%, +1.8%]
  • ✔️010_tpch_12: 62.5ms -> 62.3ms [-1.21%, +0.74%]
  • ✔️010_tpch_13: 53.3ms -> 53.3ms [-5.17%, +4.94%]
  • ✔️010_tpch_14: 42.6ms -> 41.3ms [-8.92%, +2.85%]
  • ✔️010_tpch_15: 61.6ms -> 59.8ms [-9.99%, +3.88%]
  • ✔️010_tpch_16: 44.6ms -> 45.1ms [-3.12%, +5.67%]
  • ✔️010_tpch_17: 56.1ms -> 54.8ms [-5.95%, +1.22%]
  • ✔️010_tpch_18: 56.4ms -> 54.7ms [-10.12%, +3.87%]
  • 🚀010_tpch_19: 124ms -> 122ms [-2.54%, -0.13%]
  • 🚀010_tpch_20: 73.3ms -> 69.9ms [-7.66%, -1.74%]
  • 🚀010_tpch_21: 416ms -> 381ms [-10.45%, -5.98%]
  • ✔️010_tpch_22: 78.6ms -> 77ms [-5.42%, +1.21%]
  • ❗🐌100_tpch_01: 1.17s -> 1.23s [+3.55%, +6.63%]
  • ✔️100_tpch_02: 121ms -> 122ms [-2.75%, +3.73%]
  • ✔️100_tpch_03: 1.07s -> 1.06s [-2.36%, +1.16%]
  • ✔️100_tpch_04: 1.04s -> 1.03s [-1.97%, +0.14%]
  • ✔️100_tpch_05: 1.16s -> 1.14s [-5.42%, +2.87%]
  • ✔️100_tpch_06: 984ms -> 987ms [-2.55%, +3.25%]
  • ✔️100_tpch_07: 1.11s -> 1.12s [-1.95%, +3.28%]
  • ✔️100_tpch_08: 1.14s -> 1.13s [-3.62%, +1.58%]
  • ✔️100_tpch_09: 1.21s -> 1.23s [-2.13%, +5.57%]
  • ✔️100_tpch_10: 1.09s -> 1.09s [-2.47%, +2.23%]
  • ✔️100_tpch_11: 81.1ms -> 81.6ms [-0.76%, +2.12%]
  • ✔️100_tpch_12: 1.06s -> 1.06s [-0.85%, +1.64%]
  • ✔️100_tpch_13: 312ms -> 310ms [-7.83%, +6.75%]
  • ✔️100_tpch_14: 1s -> 1s [-2.41%, +1.91%]
  • ✔️100_tpch_15: 1.09s -> 1.09s [-4.41%, +3.59%]
  • ✔️100_tpch_16: 132ms -> 127ms [-14.37%, +7.37%]
  • ✔️100_tpch_17: 1.06s -> 1.06s [-2.39%, +2.61%]
  • ✔️100_tpch_18: 1.11s -> 1.08s [-5.98%, +1.95%]
  • ✔️100_tpch_19: 1.18s -> 1.2s [-5.65%, +8.42%]
  • ✔️100_tpch_20: 1.06s -> 1.06s [-2.22%, +2.19%]
  • ✔️100_tpch_21: 2.24s -> 2.27s [-3.61%, +6.26%]
  • ✔️100_tpch_22: 174ms -> 179ms [-7.45%, +12.96%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@maelle maelle force-pushed the docs-telemetry branch 2 times, most recently from 59864e3 to 5e02fed Compare November 15, 2024 13:43
@maelle
Copy link
Collaborator Author

maelle commented Nov 15, 2024

verbose by default is horrible in the tests

Copy link
Member

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

What issue is this PR related to?

Why are we now changing to configuration based on the existence of files?

#' and thus require a higher value to enable logging.
#' Set to 99 to enable logging for all future versions.
#' Use [usethis::edit_r_environ()] to edit the environment file.
#' - To opt the current machine out of logging, run [`fallback_logging_optout()`].
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#' - To opt the current machine out of logging, run [`fallback_logging_optout()`].
#' - To opt out of logging, run [`fallback_logging_optout()`].

#' Use [usethis::edit_r_environ()] to edit the environment file.
#' - To opt the current machine out of logging, run [`fallback_logging_optout()`].
#'
#' - To opt in again for logging, run [`fallback_logging_optin()`].
#'
#' - \code{DUCKPLYR_FALLBACK_VERBOSE} controls printing, set it
Copy link
Member

Choose a reason for hiding this comment

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

Are you rewriting this part as well?

#' and thus require a higher value to enable uploading.
#' Set to 99 to enable uploading for all future versions.
#' Use [usethis::edit_r_environ()] to edit the environment file.
#' - To opt the current machine in automatic report uploads, run [`fallback_reporting_optin()`].
Copy link
Member

Choose a reason for hiding this comment

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

It's not the machine, it's the user config:

Suggested change
#' - To opt the current machine in automatic report uploads, run [`fallback_reporting_optin()`].
#' - To opt in to automatic report uploads, run [`fallback_reporting_optin()`].

} else if (fallback_uploading) {
c("v" = "Fallback uploading is enabled.")
c("v" = "Fallback automatic uploading is on. Disable it with {.fun duckplyr::fallback_reporting_optout}.")
Copy link
Member

Choose a reason for hiding this comment

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

Automatic fallback?

@@ -141,19 +133,6 @@ fallback_txt_help <- function() {
)
}

fallback_nudge <- function(call_data) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we losing this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to simplify. We advertise in other places and if the docs are clearer (they are not clearer yet) then no nudge is needed.

@maelle
Copy link
Collaborator Author

maelle commented Nov 15, 2024

@krlmlr I updated the first comment with two issue references.

Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if de9a6c6 is merged into main:

  • ✔️001_tpch_01: 24.6ms -> 25ms [-2.66%, +5.68%]
  • 🚀001_tpch_02: 135ms -> 66.4ms [-52.41%, -49.65%]
  • 🚀001_tpch_03: 72.7ms -> 44.6ms [-40.97%, -36.33%]
  • ✔️001_tpch_04: 25.7ms -> 24ms [-14.8%, +2.12%]
  • 🚀001_tpch_05: 128ms -> 61.3ms [-53.84%, -50.16%]
  • ✔️001_tpch_06: 16.3ms -> 15.7ms [-7.29%, +0.56%]
  • 🚀001_tpch_07: 160ms -> 83.9ms [-49.74%, -45.29%]
  • 🚀001_tpch_08: 175ms -> 108ms [-42.6%, -34.57%]
  • 🚀001_tpch_09: 143ms -> 73.1ms [-50.51%, -47.14%]
  • 🚀001_tpch_10: 80.2ms -> 52.4ms [-37.34%, -32.11%]
  • 🚀001_tpch_11: 65.5ms -> 39.1ms [-41.53%, -38.93%]
  • ✔️001_tpch_12: 29.4ms -> 29.7ms [-4.46%, +6.62%]
  • ✔️001_tpch_13: 20.8ms -> 21ms [-2.82%, +4.55%]
  • ✔️001_tpch_14: 22.7ms -> 22.1ms [-7.99%, +2.61%]
  • 🚀001_tpch_15: 66.9ms -> 38.8ms [-45.31%, -38.8%]
  • 🚀001_tpch_16: 72.3ms -> 42.6ms [-43.84%, -38.32%]
  • ✔️001_tpch_17: 28.3ms -> 28.1ms [-3.5%, +2.01%]
  • ✔️001_tpch_18: 24ms -> 24ms [-2.37%, +3.14%]
  • 🚀001_tpch_19: 144ms -> 72.9ms [-50.72%, -47.79%]
  • 🚀001_tpch_20: 86.4ms -> 56.1ms [-37.71%, -32.49%]
  • 🚀001_tpch_21: 189ms -> 86.9ms [-57.22%, -50.91%]
  • 🚀001_tpch_22: 139ms -> 72.2ms [-49.63%, -46.59%]
  • ✔️010_tpch_01: 84.9ms -> 83.4ms [-7.78%, +4.41%]
  • ✔️010_tpch_02: 71.4ms -> 70.2ms [-3.44%, +0.06%]
  • 🚀010_tpch_03: 70.5ms -> 67.1ms [-8.97%, -0.84%]
  • ✔️010_tpch_04: 51.3ms -> 51.4ms [-4.58%, +5.1%]
  • ✔️010_tpch_05: 95.8ms -> 94.9ms [-2.69%, +0.85%]
  • ✔️010_tpch_06: 35.9ms -> 36.7ms [-3.45%, +8.11%]
  • ✔️010_tpch_07: 116ms -> 118ms [-1.69%, +4.24%]
  • ✔️010_tpch_08: 137ms -> 137ms [-3.82%, +3.77%]
  • ✔️010_tpch_09: 120ms -> 118ms [-3.49%, +1.16%]
  • ✔️010_tpch_10: 85.7ms -> 88.4ms [-5.4%, +11.78%]
  • ✔️010_tpch_11: 40.5ms -> 39.8ms [-7.73%, +4.21%]
  • ✔️010_tpch_12: 63.3ms -> 62.8ms [-1.61%, +0.04%]
  • ✔️010_tpch_13: 54.5ms -> 55.2ms [-0.66%, +3.17%]
  • ✔️010_tpch_14: 43.1ms -> 42.7ms [-2.4%, +0.76%]
  • ✔️010_tpch_15: 59.4ms -> 61.5ms [-2.63%, +9.45%]
  • ✔️010_tpch_16: 46.5ms -> 46.4ms [-3.55%, +3.31%]
  • ✔️010_tpch_17: 58.2ms -> 58.3ms [-5.57%, +6.04%]
  • ✔️010_tpch_18: 56.7ms -> 56.4ms [-6.98%, +6.02%]
  • ✔️010_tpch_19: 124ms -> 124ms [-1.65%, +0.61%]
  • ✔️010_tpch_20: 72.6ms -> 72ms [-4.04%, +2.28%]
  • 🚀010_tpch_21: 432ms -> 403ms [-9.12%, -4.36%]
  • ✔️010_tpch_22: 82.3ms -> 80ms [-7.04%, +1.57%]
  • ✔️100_tpch_01: 1.25s -> 1.22s [-7.09%, +3.43%]
  • ✔️100_tpch_02: 122ms -> 127ms [-6.38%, +14.5%]
  • ✔️100_tpch_03: 1.15s -> 1.12s [-7.39%, +2.72%]
  • ✔️100_tpch_04: 1.13s -> 1.1s [-7.89%, +3.55%]
  • ✔️100_tpch_05: 1.23s -> 1.25s [-7.92%, +11.36%]
  • ✔️100_tpch_06: 1.05s -> 1.03s [-6.33%, +1.81%]
  • ✔️100_tpch_07: 1.19s -> 1.17s [-4.32%, +0.82%]
  • ✔️100_tpch_08: 1.21s -> 1.24s [-3.54%, +7.8%]
  • ✔️100_tpch_09: 1.29s -> 1.27s [-7.24%, +4.45%]
  • ✔️100_tpch_10: 1.16s -> 1.19s [-1.5%, +6.8%]
  • ✔️100_tpch_11: 84.7ms -> 94.5ms [-11.85%, +35.01%]
  • ✔️100_tpch_12: 1.15s -> 1.15s [-2.97%, +4.08%]
  • ✔️100_tpch_13: 334ms -> 322ms [-13.69%, +6.49%]
  • ✔️100_tpch_14: 1.05s -> 1.08s [-2%, +7.59%]
  • ✔️100_tpch_15: 1.19s -> 1.17s [-8.69%, +4.76%]
  • ✔️100_tpch_16: 130ms -> 129ms [-9.5%, +8.05%]
  • ✔️100_tpch_17: 1.14s -> 1.11s [-9.79%, +3.28%]
  • ✔️100_tpch_18: 1.16s -> 1.14s [-6.62%, +2.77%]
  • ✔️100_tpch_19: 1.24s -> 1.27s [-3.42%, +9.22%]
  • ✔️100_tpch_20: 1.15s -> 1.13s [-5.73%, +3.25%]
  • ✔️100_tpch_21: 2.29s -> 2.32s [-4.21%, +6.15%]
  • ✔️100_tpch_22: 172ms -> 177ms [-2.41%, +8.61%]

Further explanation regarding interpretation and methodology can be found in the documentation.

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.

Notes on telemetry Easier way to enable fallback reporting?
2 participants