Skip to content

feat(capture): warn when a non-string session or window id is dropped - #285

Open
eli-r-ph wants to merge 1 commit into
v1-options-parityfrom
v1-session-id-drop-warning
Open

eli-r-ph wants to merge 1 commit into
v1-options-parityfrom
v1-session-id-drop-warning

Conversation

@eli-r-ph

Copy link
Copy Markdown
Contributor

💡 Motivation and Context

Capture v1 requires $session_id and $window_id to be strings and rejects the whole request otherwise, so every v1 SDK lifts only a string and drops any other value. The drop was silent in all three SDKs. They now agree to log a warning on every drop.

  • CaptureEvent::from_event_at lifts both keys through lift_string_property. A string, including "", is sent unchanged.
  • A bool, number, array or object value is dropped with posthog-rs: dropping $session_id: a number value is not a string. The warning names the key and the type, never the value.
  • null counts as unset and drops without a warning, the same rule options use.
  • There is no warn-once state, so a caller that always sends a non-string id gets one warning per event.
  • The migration guide gets a "Session and window IDs" section.

Stacked on #279, which also changes from_event_at.

💚 How did you test it?

  • event_session_window_lift_only_strings captures tracing output for string, empty string, null, number, bool, array and object values. It checks the lifted value, that both keys leave properties, the warning text per key and type, that no value text reaches the log, and that strings and null log nothing.
  • cargo test --workspace, cargo test --no-default-features (with and without error-tracking), cargo clippy -- -D warnings, cargo fmt --check and scripts/check-public-api.sh pass locally.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

If releasing new changes

  • Ran sampo add to generate a changeset file

Human-driven (agent-assisted).

This branch has not been deployed

No deployments
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.

1 participant