opentelemetry-js-contrib
opentelemetry-js-contrib copied to clipboard
feat: Add page view instrumentation plugin
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 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.
@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 , 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
I'm sorry I think we just dropped the ball on this. If you fix the conflicts I can get this merged
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.
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.
@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 @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?
@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 - 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?
@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 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 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 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.
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.
@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 - 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 please also resolve the conflicts to get the workflows to run.
@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.