feat: Add page view instrumentation

[Abinet18]

Part of the client sdk project, Implement PageView event instrumentation
#2372

This PR adds the page view instrumentation which sends an event when a page is loaded (as soon as the html is loaded) or when a route change happens (tracking the history push and replace states)

Tested on an example app with generic routes that use the history api

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.74%. Comparing base (b043ffb) to head (562bca8).
Report is 220 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    open-telemetry/opentelemetry-js-contrib#2386   +/-   ##
=======================================
  Coverage   90.74%   90.74%           
=======================================
  Files         156      156           
  Lines        7728     7728           
  Branches     1590     1590           
=======================================
  Hits         7013     7013           
  Misses        715      715           

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[martinkuba]

Couple small changes, but overall looks good. Failing tests need to be fixed.

[martinkuba]

@Abinet18 I have approved the PR, but forgot to mention that also these files should be updated:

• .github/component-label-map.yml
• .github/component_owners.yml

[Abinet18]

@martinkuba , when can this be merged. I would fix the conflict just before the merge time so that I don't have to do it every time the main branch changes

[williazz]

Suggestion: it might make sense to use a buffered PerformanceObserver to listen for PerformanceNavigationTimings, insead of DOMEventListener for DOMContentLoaded. I have seen users be unable to find their PageViews because eventListener needs registered at runtime before DOMContentLoaded in order to scrape any data. This problem will also be worse for those have want to install this package via CDN

[williazz]

Issue: can we also teardown the event listener for DomContentLoaded? I believe it only happens once, so there's no need for it anymore.

[dyladan]

I'm sorry I think we just dropped the ball on this. If you fix the conflicts I can get this merged

[david-luna]

@Abinet18 @martinkuba

Semantic conventions for this instrumentation are not available yet open-telemetry/semantic-conventions#1910 and one of the goals of the new Browser SIG is to have an implementation of this and other instrumentations optimised for browser.

should we put this on hold? or should we continue with the possible breaking changes that will come with the optimization?

[dyladan]

Can you please address #2386 (comment)

If you want to move forward with this anyway, I will dismiss this review.

[Abinet18]

@Abinet18 @martinkuba

Semantic conventions for this instrumentation are not available yet open-telemetry/semantic-conventions#1910 and one of the goals of the new Browser SIG is to have an implementation of this and other instrumentations optimised for browser.

should we put this on hold? or should we continue with the possible breaking changes that will come with the optimization?

I am ok with putting it on hold for now but just wanted to work on migrating to logs

[pichlermarc]

@Abinet18 - we discussed this in the SIG today, and we'd prefer if new instrumentation targeting the browser would go hand-in-hand with efforts to add semantic conventions for this as well. Is that something that you'd be interested in contributing to?

[martinkuba]

@Abinet18 - we discussed this in the SIG today, and we'd prefer if new instrumentation targeting the browser would go hand-in-hand with efforts to add semantic conventions for this as well. Is that something that you'd be interested in contributing to?

@pichlermarc There is already a corresponding semconv PR here
open-telemetry/semantic-conventions#1910

[dyladan]

@martinkuba so i guess this is a prototype for that semconv? Would you want to merge this as development, experimental, unreleased, or some other status?

[martinkuba]

@martinkuba so i guess this is a prototype for that semconv? Would you want to merge this as development, experimental, unreleased, or some other status?

@dyladan I am not sure about the advantage merging this as development. If we want to merge this before the semconv is merged, I guess this would be unreleased/development? But once the semconv discussion is finished and merged, then IMO this could be experimental and published for others to try.

[dyladan]

@martinkuba so i guess this is a prototype for that semconv? Would you want to merge this as development, experimental, unreleased, or some other status?

@dyladan I am not sure about the advantage merging this as development. If we want to merge this before the semconv is merged, I guess this would be unreleased/development? But once the semconv discussion is finished and merged, then IMO this could be experimental and published for others to try.

I guess it depends if you want it released or not. As a prototype it provides some value without being merged (shows what attributes can be collected and how), but until it is merged and released it is very difficult for end-users to test it and make sure it works for their use cases. With the appropriate disclaimer I think it could be merged and released, but not included in auto-instrumentations (require users to manually install and enable it), which would make it easier for users to provide feedback to the semconv. I'm asking if you think this is required/helpful for semconv or no.

[martinkuba]

I'm asking if you think this is required/helpful for semconv or no.

Yes, I think this is helpful to get feedback from users on the semantic conventions. I am in favor of releasing this as experimental.

[pichlermarc]

@Abinet18 - looks like the build is failing - would you mind having a look? Other than that this looks like it's good to merge.

[Abinet18]

@Abinet18 - looks like the build is failing - would you mind having a look? Other than that this looks like it's good to merge.

@pichlermarc not sure why npm run lint is failing here, I can run it successfully in local

[martinkuba]

I have previously approved this. However, the semantic conventions are still not merged and will be different from what is implemented in this PR (body fields vs attributes, human-readable enum values for the type field). We can merge this now either as a development package (private set to true in package.json). Or, we can wait for the semconv PR to be merged first.

[pichlermarc]

@Abinet18 please also resolve the conflicts to get the workflows to run.