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

Success markers telemetry #10065

Merged
merged 20 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
1 change: 1 addition & 0 deletions changelog/10065.misc.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add telemetry to markers extraction.
76 changes: 76 additions & 0 deletions docs/docs/telemetry/events.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"Model Training",
"Model Testing",
"Model Serving",
"Markers Extraction",
"Data Handling",
"Miscellaneous"
],
Expand Down Expand Up @@ -422,6 +423,81 @@
"num_synonyms",
"num_regexes"
]
},
"Markers Extraction Initiated": {
"description": "Triggered when markers extraction has been initiated.",
"type": "object",
"section": "Markers Extraction",
"properties": {
"strategy": {
"type": "string",
"description": "Strategy to use when selecting trackers to extract from."
},
"only_extract": {
"type": "boolean",
"description": "Indicates if path to write out statistics hasn't been specified."
},
"seed": {
"type": ["string", "null"],
"description": "The seed to initialise the random number generator for use with the 'sample' strategy."
},
"count": {
"type": ["string", "null"],
"description": "Number of trackers to extract from (for any strategy except 'all')."
}
},
"additionalProperties": false,
"required": [
"strategy",
"only_extract",
"seed",
"count"
Comment on lines +450 to +454
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 are all of these required? 🤔 or just the top two?

Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of telemetry I think it makes sense - we'd want to know if it's being used or not. The types of those are string or null so if it's not present we'd see an explicit null. Would be good to check with someone who understands the telemetry schema here better (though I think this is only used in the docs to explain what data we send back to users and isn't used to actually validate anything internally)

]
},
"Markers Extracted": {
"description": "Triggered when markers have been extracted.",
"type": "object",
"section": "Markers Extraction",
"properties": {
"trackers_count": {
"type": "integer",
"description": "Number of processed trackers."
}
},
"additionalProperties": false,
"required": [
"trackers_count"
]
},
"Markers Parsed": {
"description": "Triggered when markers have been successfully parsed.",
"type": "object",
"section": "Markers Extraction",
"properties": {
"marker_count": {
"type": "integer",
"description": "Number of parsed markers."
}
},
"additionalProperties": false,
"required": [
"marker_count"
]
},
"Markers Stats Computed": {
"description": "Triggered when marker stats has been computed.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Triggered when marker stats has been computed.",
"description": "Triggered when marker stats have been computed.",

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Triggered when marker stats has been computed.",
"description": "Triggered when marker statistics have been computed.",

Copy link
Contributor

@aeshky aeshky Nov 3, 2021

Choose a reason for hiding this comment

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

Let's use the full word in text, and reserve "stats" for code :)

"type": "object",
"section": "Markers Extraction",
"properties": {
"trackers_count": {
"type": "integer",
"description": "Number of processed trackers."
}
},
"additionalProperties": false,
"required": [
"trackers_count"
]
}
}
}
1 change: 1 addition & 0 deletions rasa/cli/arguments/evaluate.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ 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
12 changes: 12 additions & 0 deletions rasa/cli/evaluate.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from typing import List, Text, Optional
from pathlib import Path

from rasa import telemetry
from rasa.core.utils import AvailableEndpoints
from rasa.core.tracker_store import TrackerStore
from rasa.core.evaluation.marker_tracker_loader import MarkerTrackerLoader
Expand Down Expand Up @@ -131,6 +132,13 @@ def _run_markers(
computed per session will be stored in
'<path-to-stats-folder>/statistics-per-session.csv'.
"""
telemetry.track_markers_extraction_initiated(
strategy=strategy,
only_extract=stats_file_prefix is not None,
seed=seed,
count=count,
)

domain = Domain.load(domain_path) if domain_path else None
markers = Marker.from_path(config)
if domain and not markers.validate_against_domain(domain):
Expand All @@ -139,6 +147,10 @@ def _run_markers(
"Please see errors listed above and fix before running again."
)

# 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

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

telemetry.track_markers_parsed_count(num_markers)

tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed)

def _append_suffix(path: Optional[Path], suffix: Text) -> Optional[Path]:
Expand Down
6 changes: 6 additions & 0 deletions rasa/core/evaluation/marker_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from rasa.shared.data import is_likely_yaml_file
from rasa.shared.exceptions import InvalidConfigException, RasaException
from rasa.shared.core.events import ActionExecuted, UserUttered, Event
from rasa import telemetry
from rasa.shared.core.domain import Domain
from rasa.shared.core.trackers import DialogueStateTracker
from rasa.utils.io import WriteRow
Expand Down Expand Up @@ -608,6 +609,9 @@ def evaluate_trackers(
if tracker:
tracker_result = self.evaluate_events(tracker.events)
processed_trackers[tracker.sender_id] = tracker_result

processed_trackers_count = len(processed_trackers)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really want the number of processed trackers or processed sessions (or both)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case processed trackers is actually useful because we get it before, as a command line argument (the users intent) and after (what they actually got). Might help to highlight issues in our models of how tracker stores work. Right now we don't collect session info anywhere so I'm not sure what info we could get from it, but we could also just collect it as well if you think it would be useful

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no clue how people use sender_ids and trackers usually, so no idea if that is better - but I guess it won't hurt if we don't add this now (and maybe later)

telemetry.track_markers_extracted(processed_trackers_count)
Marker._save_results(output_file, processed_trackers)

# Compute and write statistics if requested.
Expand All @@ -622,6 +626,8 @@ def evaluate_trackers(
session_idx=session_idx,
meta_data_on_relevant_events_per_marker=session_result,
)

telemetry.track_markers_stats_computed(processed_trackers_count)
if overall_stats_file:
stats.overall_statistic_to_csv(path=overall_stats_file)
if session_stats_file:
Expand Down
65 changes: 65 additions & 0 deletions rasa/telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@
TELEMETRY_VISUALIZATION_STARTED_EVENT = "Story Visualization Started"
TELEMETRY_TEST_CORE_EVENT = "Model Core Tested"
TELEMETRY_TEST_NLU_EVENT = "Model NLU Tested"
TELEMETRY_MARKERS_EXTRACTION_INITIATED_EVENT = "Markers Extraction Initiated"
TELEMETRY_MARKERS_EXTRACTED_EVENT = "Markers Extracted"
TELEMETRY_MARKERS_STATS_COMPUTED_EVENT = "Markers Stats Computed"
TELEMETRY_MARKERS_PARSED_COUNT = "Markers Parsed"

# used to calculate the context on the first call and cache it afterwards
TELEMETRY_CONTEXT = None
Expand Down Expand Up @@ -1005,3 +1009,64 @@ def track_nlu_model_test(test_data: "TrainingData") -> None:
"num_regexes": len(test_data.regex_features),
},
)


@ensure_telemetry_enabled
def track_markers_extraction_initiated(
strategy: Text, only_extract: bool, seed: Optional[int], count: Optional[int],
) -> None:
"""Track when a user tries to extract success markers.

Args:
strategy: The strategy the user is using for tracker selection
only_extract: Indicates if the user is only extracting markers or also
producing stats
seed: (Optional) The seed used if strategy is 'sample' and the user selects
one
count: (Optional) The number of trackers the user is trying to select.
"""
_track(
TELEMETRY_MARKERS_EXTRACTION_INITIATED_EVENT,
{
"strategy": strategy,
"only_extract": only_extract,
"seed": seed,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will report the actual seed used by the user - do we want the actual seed or do we want to just know if they used a seed? Is this something that's worth changing or are we considering the actual seed value not important enough to be careful about not collecting?

Copy link
Contributor

Choose a reason for hiding this comment

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

We only want to know if the user set a seed. I don't think it counts as private info so maybe it doesn't matter if we collect it. But if we can do true/false that might be better.

"count": count,
},
)


@ensure_telemetry_enabled
def track_markers_extracted(trackers_count: int) -> None:
"""Track when markers have been extracted by a user.

Args:
trackers_count: The actual number of trackers processed
"""
_track(
TELEMETRY_MARKERS_EXTRACTED_EVENT, {"trackers_count": trackers_count},
)


@ensure_telemetry_enabled
def track_markers_stats_computed(trackers_count: int) -> None:
"""Track when stats over markers have been computed by a user.

Args:
trackers_count: The actual number of trackers processed
"""
_track(
TELEMETRY_MARKERS_STATS_COMPUTED_EVENT, {"trackers_count": trackers_count},
)


@ensure_telemetry_enabled
def track_markers_parsed_count(marker_count: int) -> None:
"""Track when markers have been successfully parsed from config.

Args:
marker_count: The number of markers found in the config
"""
_track(
TELEMETRY_MARKERS_PARSED_COUNT, {"marker_count": marker_count},
)
8 changes: 7 additions & 1 deletion tests/test_telemetry.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,16 @@ async def test_events_schema(

telemetry.track_nlu_model_test(TrainingData())

telemetry.track_markers_evaluation_initiated("all", False, None, None)

telemetry.track_markers_extracted(1)

telemetry.track_markers_stats_computed(1)

pending = asyncio.all_tasks() - initial
await asyncio.gather(*pending)

assert mock.call_count == 15
assert mock.call_count == 18

for args, _ in mock.call_args_list:
event = args[0]
Expand Down