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: report requests even without timing #1386

Merged
merged 7 commits into from
Sep 3, 2024

Conversation

pauldambra
Copy link
Member

@pauldambra pauldambra commented Aug 26, 2024

related to https://posthoghelp.zendesk.com/agent/tickets/16517

In that ticket xhr calls to an API are not being recorded.

ultimately this is because if there is no performance entry reported by the browser we don't send any information back to posthog :/ no bueno

In that site I can capture those calls with a performance observer by pasting the following in the terminal

// Function to log performance entries
function logPerformanceEntries(list) {
    const entries = list.getEntries();
    entries.forEach((entry) => {
        console.log('Performance entry:', entry);
    });
}

// Create a PerformanceObserver instance
const observer = new PerformanceObserver(logPerformanceEntries);

// Observe the 'resource' type entries which include fetch and XHR requests
observer.observe({ type: 'resource', buffered: true });

that's not quite how the recorder works but should be equivalent

but when we try to get the call in the recorder none is returned

i can't see any code in the site calling performance.clearResourceTimings()

but the existence of performance.clearResourceTimings means we can't guarantee timings are available anyway.

so we should report what we can


maybe we should

  • roll this out behind config so we can check the impact on posthog and just the reporters site at first to check we're not breaking display assumptions

tested locally and can still capture performance data and network payloads/headers

Copy link

vercel bot commented Aug 26, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Sep 3, 2024 1:28pm

Copy link

github-actions bot commented Aug 26, 2024

Size Change: +912 B (+0.08%)

Total Size: 1.18 MB

Filename Size Change
dist/array.full.js 335 kB +304 B (+0.09%)
dist/recorder-v2.js 110 kB +304 B (+0.28%)
dist/recorder.js 110 kB +304 B (+0.28%)
ℹ️ View Unchanged
Filename Size
dist/array.js 156 kB
dist/exception-autocapture.js 10.4 kB
dist/main.js 157 kB
dist/module.js 156 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.26 kB
dist/web-vitals.js 5.79 kB

compressed-size-action

src/entrypoints/recorder.ts Outdated Show resolved Hide resolved
src/entrypoints/recorder.ts Show resolved Hide resolved
Comment on lines +362 to +363
start?: number
end?: number
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to make these optional? Looks like this method is only called with the results of window.performance.now() as inputs for these values which always returns a number

Copy link
Member Author

Choose a reason for hiding this comment

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

only that the values start undefined... really you shouldn't ever get to the point of needing them and the value not be present but 🙈

@pauldambra pauldambra marked this pull request as ready for review August 30, 2024 07:44
@pauldambra pauldambra changed the title feat: report requests even without timing fix: report requests even without timing Aug 30, 2024
@pauldambra pauldambra added the bump patch Bump patch version when this PR gets merged label Aug 30, 2024
Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Is there a test we could write that would simulate the missing performance entry?

@daibhin
Copy link
Contributor

daibhin commented Aug 30, 2024

(Separately, but somewhat related to this change)

I was looking at the reported issue where the performance events were missing for a customer. Given we initialise our own PerformanceObserver maybe we could store the events it captures and use that as a lookup guide instead of the performance.getEntriesByType('resource') we rely on today

@pauldambra
Copy link
Member Author

pauldambra commented Aug 30, 2024

Is there a test we could write that would simulate the missing performance entry?

playing with more testing of the network recorder at the moment...

i'm sort of worried that this suffers a common problem of this kind of "wrapping external APIs" code. either the scaffolding is complex or we split things so that we can test the behaviour and we lose all encapsulation 🙈

@pauldambra
Copy link
Member Author

Given we initialise our own PerformanceObserver maybe we could store the events it captures and use that as a lookup guide instead of the performance.getEntriesByType('resource') we rely on today

🤔

@pauldambra
Copy link
Member Author

I'll wait and get #1395 in before this so we at least have one more level of testing on top of it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants