Skip to content

Conversation

cyhk
Copy link
Contributor

@cyhk cyhk commented Sep 12, 2025

Summary

Fix tests and release page-url-enrichment-plugin to default on for customers

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@cyhk cyhk marked this pull request as ready for review September 29, 2025 16:07
Copy link

promptless bot commented Sep 29, 2025

📝 Documentation updates detected!

New suggestion: Document page URL enrichment plugin enabled by default in Browser SDK

generateSessionEndEvent(++eventId),
addPageUrlEnrichmentProperties(generateSessionStartEvent(++eventId), url),
generatePageViewEvent(++eventId, 1, url, undefined, { previousPageUrl: '' }),
addPageUrlEnrichmentProperties(generateSessionEndEvent(++eventId), directUrl, url),
Copy link
Contributor Author

@cyhk cyhk Oct 1, 2025

Choose a reason for hiding this comment

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

Tests are passing and make sense to me, except this (and other similar cases with session_end events have what is expected to be the next page's data.

When do session end events get fired?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed this now. session_end events get tracked when a user visits a website that has Amplitude tracking on it and the "lastEventTime" (which gets updated to the latest time every event) is >30 minutes in the past. When that happens, the client creates a "session_end" event and fires it with "lastEventTime + 1" as it's timestamp.

So the session end events are uploaded at least 30 minutes after the last event in the session takes place. But usually it's way after that.

@cyhk cyhk force-pushed the AMP-139733 branch 3 times, most recently from fcc7160 to 7674bda Compare October 14, 2025 20:37
Copy link
Contributor

@Mercy811 Mercy811 left a comment

Choose a reason for hiding this comment

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

Overall LGTM!

Copy link
Collaborator

@daniel-graham-amplitude daniel-graham-amplitude left a comment

Choose a reason for hiding this comment

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

A couple optional comments. Lgtm though.

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.

3 participants