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

Success markers telemetry #10065

Merged
merged 20 commits into from
Nov 5, 2021
Merged

Success markers telemetry #10065

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
2d32b83
Markers telemetry
alwx Nov 2, 2021
56fb74f
Everything without tests
alwx Nov 2, 2021
2d8b79e
Specified events.json
alwx Nov 2, 2021
4f7a6ff
Test added
alwx Nov 2, 2021
15c1c6d
Changelog entry
alwx Nov 2, 2021
fd1d7e0
Naming fixes
alwx Nov 3, 2021
c95dcde
Fix lint, fix CLI bug
usc-m Nov 3, 2021
7359438
Add markers parsed telemetry
usc-m Nov 3, 2021
1b4535a
Document telemetry functions
usc-m Nov 3, 2021
015cf4d
always add ANY_MARKER; add test
ka-bu Nov 3, 2021
bef34e8
Add complexity telemetry
usc-m Nov 3, 2021
58174e6
Skip root marker to avoid double reporting total marker count
usc-m Nov 3, 2021
4c98d1a
Unfix erroneous CLI fix, fix typos
usc-m Nov 3, 2021
1df9776
Fix event schema test
usc-m Nov 3, 2021
059be98
Fix mock count in telemetry test
ka-bu Nov 4, 2021
c617ebd
trigger test re-run
ka-bu Nov 4, 2021
b25b422
Make sure seed is converted to bool correctly
usc-m Nov 4, 2021
979ecd1
Fix telemetry test
usc-m Nov 4, 2021
442f2bf
Merge branch 'main' into markers-telemetry
usc-m Nov 4, 2021
c030770
Merge branch 'main' into markers-telemetry
aeshky Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Unfix erroneous CLI fix, fix typos
  • Loading branch information
@usc-m
usc-m committed Nov 3, 2021
commit 4c98d1ac10576de550ae9013a985645db1ef1864
6 changes: 3 additions & 3 deletions docs/docs/telemetry/events.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@
]
},
"Markers Extraction Initiated": {
"description": "Triggered when markers extraction has been initiated.",
"description": "Triggered when marker extraction has been initiated.",
"type": "object",
"section": "Markers Extraction",
"properties": {
Expand Down Expand Up @@ -494,8 +494,8 @@
"branching_factor"
]
},
"Markers Stats Computed": {
"description": "Triggered when marker stats has been computed.",
"Markers Statistics Computed": {
"description": "Triggered when marker statistics have been computed.",
"type": "object",
"section": "Markers Extraction",
"properties": {
Expand Down
1 change: 0 additions & 1 deletion rasa/cli/arguments/evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ def set_markers_arguments(parser: argparse.ArgumentParser) -> None:
parser.add_argument(
"--config",
default="markers.yml",
nargs="?",
type=str,
help="The config file(s) containing marker definitions. This can be a single "
"YAML file, or a directory that contains several files with marker "
Expand Down
2 changes: 1 addition & 1 deletion rasa/cli/evaluate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ def _run_markers(
# Calculate telemetry
# Subtract one to remove the virtual OR over all markers
num_markers = len(markers) - 1
Copy link
Contributor

@ka-bu ka-bu Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

this is the total number of all conditions and operators used in all marker configurations that were evaluated -- to get the number of user-defined markers we should

  1. get rid of the special case here and always add an "ANY_MARKER
  2. len(markers.sub_markers) instead of len(markers) ... My bad, iter gives us all conditions and operators but len is just the sub-markers all good 👍 - but because of 1. there might not be sub-markers if there is a single user defined marker

Copy link
Contributor

Choose a reason for hiding this comment

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

@usc-m , I could help and add that while you work on other comments if you like?

Copy link
Contributor

Choose a reason for hiding this comment

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

we could also track something like max( len(sub_marker) for sub_marker in markers), i.e. the maximum number of conditions and operators used in a single user defined marker, to get a glimpse of how complex the queries are

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I suggested that too, also the branching factor (maximum number of children under any marker)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh yeah, that's also a nice idea -- will definitely tell us if people miss begin able to re-use a marker definition

max_depth = markers.depth() - 1
max_depth = markers.max_depth() - 1
# Find maximum branching of marker
branching_factor = max(
len(sub_marker) - 1 for marker in markers.sub_markers for sub_marker in marker
Expand Down