diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 63ff960..e9fa019 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -181,7 +181,18 @@ This is now the shipped behavior, not just the target. targets, and existing stored profiles are periodically re-validated against current observed shapes (flagged for review, never silently rewritten — consistent with the drift-flagging precedent already used for - auto-applied field mappings). + auto-applied field mappings). Turning this on live surfaced a real, + pre-existing calibration bug in `classify_field_shape()`: callers pass a + *deduplicated* set of observed values (capped at 200 distinct entries), + whose own length was being used as the cardinality denominator — since a + deduplicated set is always 100% cardinality against itself, any field with + more than ~16 distinct values looked "high cardinality" regardless of true + traffic volume, misclassifying genuinely low-cardinality fields like + `username` (82 distinct values out of 1,601 events) as free text. Fixed by + passing the real event coverage (`coverage[field]`) as an explicit + `total_events` parameter, so cardinality is judged against actual volume; + omitting it preserves the old sample-as-population behavior for other + callers. - **Shared, entity-scoped baselines** for entities that shared-field discovery already recognizes as the same concept across multiple enabled streams, instead of one cold baseline per `(stream, entity, field)` tuple. diff --git a/src/fgai/field_shapes.py b/src/fgai/field_shapes.py index 5d02d8e..d8f5815 100644 --- a/src/fgai/field_shapes.py +++ b/src/fgai/field_shapes.py @@ -88,11 +88,21 @@ def _name_bonus(field_name: str, shape: str) -> int: return NAME_BONUS_CAP if any(token in lower for token in tokens) else 0 -def classify_field_shape(sample_values, *, field_name: str = "") -> FieldShape: +def classify_field_shape(sample_values, *, field_name: str = "", total_events: int | None = None) -> FieldShape: """Classify a field's semantic shape from its observed values. Value shape is the primary signal; ``field_name`` only contributes a small, capped bonus that can nudge an already-plausible shape, never manufacture one. + + ``sample_values`` is commonly a deduplicated set of distinct observed values + (callers cap how many distinct values they remember per field), so its own + length is not a usable stand-in for "how many events had this field" — a + deduplicated collection is trivially 100% cardinality against itself. Pass + the real event count as ``total_events`` when available (for example + ``coverage[field]``) so cardinality is judged against real traffic volume; + otherwise this falls back to treating the sample itself as the population, + which under-detects genuinely low-cardinality fields once they have more + than a handful of distinct values. """ values = [str(item).strip() for item in sample_values if str(item).strip()] if not values: @@ -100,7 +110,8 @@ def classify_field_shape(sample_values, *, field_name: str = "") -> FieldShape: total = len(values) cardinality = len(set(values)) - low_cardinality = cardinality <= max(10, int(total * 0.5)) + population = total_events if total_events and total_events > total else total + low_cardinality = cardinality <= max(10, int(population * 0.5)) ip_ratio = sum(_is_ip(v) for v in values) / total timestamp_ratio = sum(_is_timestamp(v) for v in values) / total @@ -109,8 +120,8 @@ def classify_field_shape(sample_values, *, field_name: str = "") -> FieldShape: non_numeric_ratio = 1 - numeric_ratio avg_words = sum(len(v.split()) for v in values) / total multiword = avg_words >= 3 - high_cardinality = cardinality > max(8, int(total * 0.5)) - categorical_ratio = non_numeric_ratio if (1 < cardinality <= max(8, int(total * 0.5)) and not multiword) else 0.0 + high_cardinality = cardinality > max(8, int(population * 0.5)) + categorical_ratio = non_numeric_ratio if (1 < cardinality <= max(8, int(population * 0.5)) and not multiword) else 0.0 free_text_ratio = 0.4 if ((high_cardinality or multiword) and numeric_ratio < 0.5) else 0.0 fit_by_shape = { diff --git a/src/fgai/profile_suggestions.py b/src/fgai/profile_suggestions.py index ea92678..eedb913 100644 --- a/src/fgai/profile_suggestions.py +++ b/src/fgai/profile_suggestions.py @@ -957,7 +957,7 @@ def suggest_stream_profiles(events: list[LogEvent], *, existing_profiles: dict[s for field in coverage: if field in IGNORED_DISCOVERY_FIELDS: continue - shape = classify_field_shape(unique_values[field], field_name=field) + shape = classify_field_shape(unique_values[field], field_name=field, total_events=coverage[field]) field_shapes[field] = shape.primary_shape field_confidence[field] = shape.confidence shared_categorical, shared_numeric, shared_stats = _shared_profile_fields(coverage, unique_values, numeric_counts, total, stream_field_counts, stream_semantic_counts, stream_semantic_fields) diff --git a/tests/test_field_shapes.py b/tests/test_field_shapes.py index 20ba46f..a496a9b 100644 --- a/tests/test_field_shapes.py +++ b/tests/test_field_shapes.py @@ -63,6 +63,25 @@ class FieldShapesTests(unittest.TestCase): with_name = classify_field_shape(values, field_name="srcip") self.assertLessEqual(with_name.confidence - without_name.confidence, 15) + def test_deduplicated_sample_alone_misjudges_cardinality_without_total_events(self): + # sample_values is commonly a deduplicated set (callers cap distinct values + # remembered per field), so its own length can't stand in for real event + # volume: 82 distinct plain usernames look "high cardinality" against + # themselves even though they're a tiny fraction of a much larger stream. + values = [f"user{n}" for n in range(82)] + without_total = classify_field_shape(values, field_name="username") + self.assertEqual(without_total.primary_shape, "free_text") + + def test_total_events_corrects_cardinality_judgement_for_low_cardinality_field(self): + values = [f"user{n}" for n in range(82)] + result = classify_field_shape(values, field_name="username", total_events=880) + self.assertEqual(result.primary_shape, "categorical_low_cardinality") + + def test_total_events_does_not_rescue_genuinely_high_cardinality_field(self): + values = [f"user{n}" for n in range(82)] + result = classify_field_shape(values, field_name="username", total_events=95) + self.assertEqual(result.primary_shape, "free_text") + def test_empty_values_return_zero_confidence(self): result = classify_field_shape([], field_name="whatever") self.assertEqual(result.confidence, 0) diff --git a/tests/test_profile_suggestions.py b/tests/test_profile_suggestions.py index cbc1d1a..0732f90 100644 --- a/tests/test_profile_suggestions.py +++ b/tests/test_profile_suggestions.py @@ -393,6 +393,20 @@ class ProfileSuggestionTests(unittest.TestCase): self.assertNotIn("context", relationship_fields) self.assertIn("srcip", profile["entity_fields"]) + def test_field_shape_classification_uses_real_event_volume_not_sample_size(self): + # 24 distinct usernames repeating across 300 events is genuinely low + # cardinality (8%) even though 24 distinct values alone would look + # "high cardinality" if judged only against the deduplicated sample. + events = [ + parse_log_line(f"fgai_stream_id=app fgai_stream=App username=user{index % 24} action=login timestamp=2026-06-29T10:00:{index % 60:02d}Z") + for index in range(1, 301) + ] + + suggestion = suggest_stream_profiles(events)[0] + + self.assertEqual(suggestion["field_shapes"]["username"], "categorical_low_cardinality") + self.assertIn("username", suggestion["profile"]["entity_fields"]) + def test_low_cardinality_name_matched_field_is_not_vetoed(self): events = [ parse_log_line(