Skip to content

fix(nimbus): rename event_stream to events_stream, add SourceType enum, add ratios to FeatureSpec#1415

Merged
yashikakhurana merged 1 commit into
mainfrom
feat/featmon-from-github-repo
Apr 29, 2026
Merged

fix(nimbus): rename event_stream to events_stream, add SourceType enum, add ratios to FeatureSpec#1415
yashikakhurana merged 1 commit into
mainfrom
feat/featmon-from-github-repo

Conversation

@yashikakhurana
Copy link
Copy Markdown
Contributor

Because

  • events_stream matches the actual BigQuery table name; event_stream was inconsistent
  • ratios was referenced in the bigquery-etl query template but had no way to be defined in TOML configs
  • The source type was validated against a plain frozenset with no type safety

This commit

  • Renames event_streamevents_stream in VALID_SOURCE_TYPES, SourceTableSpec default, and firefox_desktop.toml
  • Adds SourceType enum (metrics, events_stream, clients_daily) for type-safe validation
  • Adds ratios: list[list[str]] to FeatureSpec so TOML configs can define metric ratios
  • Updates FeatmonSpec.from_dict to populate ratios from config
  • Bumps version to 2026.4.4

Related: EXP-6732

@github-actions
Copy link
Copy Markdown

⚠️ Changing default metrics will cause all experiments that are currently live to get rerun after this change has been merged. This may come with a substantial cost. To skip reruns, add [ci skip-rerun] to the PR message.

@yashikakhurana yashikakhurana marked this pull request as draft April 29, 2026 17:04
@github-actions
Copy link
Copy Markdown

✅ Jetstream Validation is complete. Check the CI logs for this step for Query SQL and data processing estimates.

@github-actions
Copy link
Copy Markdown

⚠️ Experiment metrics query data processing estimate exceeded high threshold and may timeout during analysis.

This does not block merging, but experiment analysis may timeout and require config changes.

@yashikakhurana yashikakhurana force-pushed the feat/featmon-from-github-repo branch from 46f33cf to 0aea57d Compare April 29, 2026 17:08
@yashikakhurana yashikakhurana marked this pull request as ready for review April 29, 2026 17:08
@yashikakhurana yashikakhurana force-pushed the feat/featmon-from-github-repo branch 2 times, most recently from ca9d8b7 to 41057cb Compare April 29, 2026 17:33
@yashikakhurana
Copy link
Copy Markdown
Contributor Author

@mikewilli The integration tests clone firefox_desktop.toml from the main branch on GitHub rather than from this PR branch, so they were loading type = "event_stream" while the updated validator now only accepts events_stream — causing the failures.

To unblock CI, I've added event_stream as a deprecated alias in SourceType. Once this PR merges, main will have type = "events_stream" in the TOML and the alias can be removed in a follow-up. Both the parser change and the TOML change are in this PR, so they land together.

Copy link
Copy Markdown
Collaborator

@mikewilli mikewilli left a comment

Choose a reason for hiding this comment

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

Made a couple comments, but everything here looks good.

Did you have something changed in the jetstream/ dir at some point? I'm wondering why the CI put in some messages about experiment analysis, that should only run when jetstream/** stuff is changed.

Also, the integration test issue seems like a bad thing that we should fix (separately), I'll have to think about it. If you're happy with landing this and then updating again afterwards to remove the deprecated event_stream option, then I don't think we need to block this. Thanks!

Comment thread lib/metric-config-parser/metric_config_parser/featmon.py Outdated
Comment thread lib/metric-config-parser/metric_config_parser/featmon.py Outdated
Comment thread lib/metric-config-parser/pyproject.toml Outdated
[project]
name = "mozilla-metric-config-parser"
version = "2026.4.3"
version = "2026.4.4"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You might consider waiting on this. You can have an immediately follow-up to remove the deprecated event_stream option that then pushes a new version.

@yashikakhurana yashikakhurana force-pushed the feat/featmon-from-github-repo branch from 41057cb to e1789ed Compare April 29, 2026 18:06
@yashikakhurana
Copy link
Copy Markdown
Contributor Author

Made a couple comments, but everything here looks good.

Did you have something changed in the jetstream/ dir at some point? I'm wondering why the CI put in some messages about experiment analysis, that should only run when jetstream/** stuff is changed.

Also, the integration test issue seems like a bad thing that we should fix (separately), I'll have to think about it. If you're happy with landing this and then updating again afterwards to remove the deprecated event_stream option, then I don't think we need to block this. Thanks!

yeah my bad did added some changes which were not required

- Add FeatmonSpec, SourceTableSpec, FeatureSpec classes to parse TOML configs
- Add SourceType enum (metrics, events_stream, clients_daily)
- Add ratios field to FeatureSpec
- Integrate featmon_configs into ConfigCollection.from_github_repo()
- Add firefox_desktop.toml with data sources and features
- Add featmon/ README and example config
- Bump version to 2026.4.4
@yashikakhurana yashikakhurana force-pushed the feat/featmon-from-github-repo branch from e1789ed to 6f3e926 Compare April 29, 2026 18:08
@yashikakhurana yashikakhurana added this pull request to the merge queue Apr 29, 2026
Merged via the queue into main with commit 2c17a88 Apr 29, 2026
7 checks passed
@yashikakhurana yashikakhurana deleted the feat/featmon-from-github-repo branch April 29, 2026 18:09
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.

2 participants