-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Success markers telemetry #10065
Changes from all commits
2d32b83
56fb74f
2d8b79e
4f7a6ff
15c1c6d
fd1d7e0
c95dcde
7359438
1b4535a
015cf4d
bef34e8
58174e6
4c98d1a
1df9776
059be98
c617ebd
b25b422
979ecd1
442f2bf
c030770
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add telemetry to markers extraction. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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 is not None, | ||
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): | ||
|
@@ -139,6 +147,17 @@ def _run_markers( | |
"Please see errors listed above and fix before running again." | ||
) | ||
|
||
# Calculate telemetry | ||
# Subtract one to remove the virtual OR over all markers | ||
num_markers = len(markers) - 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we could also track something like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.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 | ||
) | ||
|
||
telemetry.track_markers_parsed_count(num_markers, max_depth, branching_factor) | ||
|
||
tracker_loader = _create_tracker_loader(endpoint_config, strategy, count, seed) | ||
|
||
def _append_suffix(path: Optional[Path], suffix: Text) -> Optional[Path]: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -271,6 +272,11 @@ def validate_against_domain(self, domain: Domain) -> bool: | |
""" | ||
... | ||
|
||
@abstractmethod | ||
def max_depth(self) -> int: | ||
"""Gets the maximum depth from this point in the marker tree.""" | ||
... | ||
|
||
def evaluate_events( | ||
self, events: List[Event], recursive: bool = False | ||
) -> List[SessionEvaluation]: | ||
|
@@ -447,12 +453,8 @@ def from_path(cls, path: Union[Path, Text]) -> Marker: | |
loaded_markers.append(marker) | ||
|
||
# combine the markers | ||
if len(loaded_markers) > 1: | ||
marker = OrMarker(markers=loaded_markers) | ||
marker.name = Marker.ANY_MARKER # cannot be set via name parameter | ||
else: | ||
marker = loaded_markers[0] | ||
|
||
marker = OrMarker(markers=loaded_markers) | ||
marker.name = Marker.ANY_MARKER # cannot be set via name parameter | ||
return marker | ||
|
||
@staticmethod | ||
|
@@ -608,6 +610,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -622,6 +627,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: | ||
|
@@ -756,6 +763,10 @@ def validate_against_domain(self, domain: Domain) -> bool: | |
marker.validate_against_domain(domain) for marker in self.sub_markers | ||
) | ||
|
||
def max_depth(self) -> int: | ||
"""Gets the maximum depth from this point in the marker tree.""" | ||
return 1 + max(child.max_depth() for child in self.sub_markers) | ||
|
||
@staticmethod | ||
def from_tag_and_sub_config( | ||
tag: Text, sub_config: Any, name: Optional[Text] = None, | ||
|
@@ -837,6 +848,10 @@ def __len__(self) -> int: | |
"""Returns the count of all markers that are part of this marker.""" | ||
return 1 | ||
|
||
def max_depth(self) -> int: | ||
"""Gets the maximum depth from this point in the marker tree.""" | ||
return 1 | ||
|
||
@staticmethod | ||
def from_tag_and_sub_config( | ||
tag: Text, sub_config: Any, name: Optional[Text] = None | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 Statistics Computed" | ||
TELEMETRY_MARKERS_PARSED_COUNT = "Markers Parsed" | ||
|
||
# used to calculate the context on the first call and cache it afterwards | ||
TELEMETRY_CONTEXT = None | ||
|
@@ -1005,3 +1009,72 @@ 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: bool, 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: Indicates if the user used a seed for this attempt | ||
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, max_depth: int, branching_factor: int | ||
) -> None: | ||
"""Track when markers have been successfully parsed from config. | ||
|
||
Args: | ||
marker_count: The number of markers found in the config | ||
max_depth: The maximum depth of any marker in the config | ||
branching_factor: The maximum number of children of any marker in the config. | ||
""" | ||
_track( | ||
TELEMETRY_MARKERS_PARSED_COUNT, | ||
{ | ||
"marker_count": marker_count, | ||
"max_depth": max_depth, | ||
"branching_factor": branching_factor, | ||
}, | ||
) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ornull
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)