opentelemetry-js-contrib icon indicating copy to clipboard operation
opentelemetry-js-contrib copied to clipboard

feat: Add page view instrumentation plugin

Open Abinet18 opened this issue 1 year ago • 3 comments

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

Abinet18 avatar Aug 16 '24 21:08 Abinet18

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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           
:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Aug 19 '24 17:08 codecov[bot]

@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

martinkuba avatar Oct 15 '24 15:10 martinkuba

@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

Abinet18 avatar Oct 31 '24 21:10 Abinet18

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

dyladan avatar May 14 '25 16:05 dyladan

This package does not have an assigned component owner and is considered unmaintained. As such this package is in feature-freeze and this PR will be closed with 14 days unless a new owner or a sponsor (a member of @open-telemetry/javascript-approvers) for the feature is found. It is the responsibility of the author to find a sponsor for this feature. Are you familiar with this package? Consider becoming a component owner.

github-actions[bot] avatar Jun 17 '25 06:06 github-actions[bot]

Looks like the rebase went wrong which made the automation ping everyone and triggered the autoclose. @Abinet18 - once rebased on top of main, please ping @opentelemetry/javascript-contrib-triagers to remove the excess labels that don't apply anymore to avoid the autoclose.

pichlermarc avatar Jun 17 '25 08:06 pichlermarc

@opentelemetry/javascript-contrib-triagers, sorry for the rebase issue that added the excess labels. I have fixed the rebase , can some one remove them. I can also close this PR creating a new one in place of it

Abinet18 avatar Jun 17 '25 19:06 Abinet18

@Abinet18 @martinkuba

Semantic conventions for this instrumentation are not available yet https://github.com/open-telemetry/semantic-conventions/pull/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?

david-luna avatar Jun 18 '25 16:06 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?

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

Abinet18 avatar Jun 25 '25 17:06 Abinet18

@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 avatar Jul 30 '25 16:07 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?

@pichlermarc There is already a corresponding semconv PR here https://github.com/open-telemetry/semantic-conventions/pull/1910

martinkuba avatar Aug 01 '25 14:08 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 avatar Aug 06 '25 16:08 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.

martinkuba avatar Aug 07 '25 09:08 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.

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.

dyladan avatar Aug 07 '25 15:08 dyladan

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.

martinkuba avatar Aug 11 '25 11:08 martinkuba

@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 avatar Aug 13 '25 16:08 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.

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

Abinet18 avatar Aug 13 '25 19:08 Abinet18

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

pichlermarc avatar Sep 10 '25 16:09 pichlermarc

@Abinet18 closing this PR as it seems like #3148 is the replacement for this PR. Please feel free to re-open if that's not the case.

pichlermarc avatar Nov 19 '25 17:11 pichlermarc