Skip to content
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

fix: posthog path to ignore #1054

Merged
merged 3 commits into from
Mar 5, 2024
Merged

fix: posthog path to ignore #1054

merged 3 commits into from
Mar 5, 2024

Conversation

marandaneto
Copy link
Member

@marandaneto marandaneto commented Mar 5, 2024

We had a typo in the code that ignored ingestion paths so we were recording i/v0/e/

well, not any more

@posthog-bot
Copy link
Collaborator

Hey @marandaneto! 👋
This pull request seems to contain no description. Please add useful context, rationale, and/or any other information that will help make sense of this change now and in the distant Mars-based future.

@@ -68,7 +68,7 @@ const removeAuthorizationHeader = (data: CapturedNetworkRequest): CapturedNetwor
return data
}

const POSTHOG_PATHS_TO_IGNORE = ['/s/', '/e/', '/i/vo/e/']
const POSTHOG_PATHS_TO_IGNORE = ['/s/', '/e/', '/i/v0/e/', '/i/vo/e/', '/decide/', '/static/recorder-v2.js']
Copy link
Member Author

Choose a reason for hiding this comment

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

was vo a typo or a valid one in the past?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

totally a typo...

let's keep it to just ingestion paths for now... others can be useful when viewing our own site

@@ -68,7 +68,7 @@ const removeAuthorizationHeader = (data: CapturedNetworkRequest): CapturedNetwor
return data
}

const POSTHOG_PATHS_TO_IGNORE = ['/s/', '/e/', '/i/vo/e/']
const POSTHOG_PATHS_TO_IGNORE = ['/s/', '/e/', '/i/v0/e/', '/i/vo/e/', '/decide/', '/static/recorder-v2.js']
Copy link
Member Author

Choose a reason for hiding this comment

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

the other option is to ignore the whole domain rather than just the paths.

Copy link
Member

Choose a reason for hiding this comment

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

i was wondering about that... once the dust has settled we should have ingestion domain and non-ingestion domains and then we can ignore the ingestion domain

Copy link

github-actions bot commented Mar 5, 2024

Size Change: +112 B (0%)

Total Size: 864 kB

Filename Size Change
dist/array.full.js 186 kB +28 B (0%)
dist/array.js 127 kB +28 B (0%)
dist/es.js 127 kB +28 B (0%)
dist/module.js 127 kB +28 B (0%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12 kB
dist/recorder-v2.js 106 kB
dist/recorder.js 58.6 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

// want to ignore posthog paths when capturing requests, or we can get trapped in a loop
// because calls to PostHog would be reported using a call to PostHog which would be reported....
const ignorePostHogPaths = (data: CapturedNetworkRequest): CapturedNetworkRequest | undefined => {
const url = convertToURL(data.name)
if (url && url.pathname && POSTHOG_PATHS_TO_IGNORE.includes(url.pathname)) {
Copy link
Member

Choose a reason for hiding this comment

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

so if someone else has /i/my-important-data then we won't capture it but let's cross that bridge when we come to it

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

me reviewing the code i changed

startrek-nodding

@pauldambra pauldambra marked this pull request as ready for review March 5, 2024 14:49
@pauldambra pauldambra merged commit 7bd2c15 into main Mar 5, 2024
14 checks passed
@pauldambra pauldambra deleted the fix/url-pattern branch March 5, 2024 15:19
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