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.

Sign up for GitHub

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

Jump to bottom

feat: add service support to analytics mechnism #1938

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: add service support to analytics mechnism #1938

wants to merge 1 commit into from

Conversation

kpanot
Copy link
Contributor

@kpanot kpanot commented Jul 1, 2024 •
edited
Loading

Proposed change

Provide a way to emit Analytics events to multiple providers:

  • Multiple provider support
  • Split of report and collect services
  • Support of simple event (not class anymore)
  • Split of Analytics Events and Performance Events
  • Implementation of Logger reporting analytics
  • Deprecation of Analytics part of the current service

Missing

  • Readme Documentation
  • In-code comments
  • Unit Tests
  • Simplied module

@kpanot kpanot requested a review from a team as a code owner July 1, 2024 09:51
@kpanot kpanot marked this pull request as draft July 1, 2024 09:51
@kpanot kpanot force-pushed the feature/analytics branch 2 times, most recently from 358291c to 584122e Compare July 1, 2024 09:54
@nx-cloud Nx Cloud
Copy link

nx-cloud bot commented Jul 1, 2024 •
edited
Loading

View your CI Pipeline Execution ↗ for commit e4af13b.

Command Status Duration Result
nx run-many --target=test-int ✅ Succeeded 55m 19s View ↗
nx run-many --target=test-e2e ✅ Succeeded 9m 4s View ↗
nx run-many --target=build --projects=eslint-pl... ✅ Succeeded <1s View ↗
nx run-many --target=publish --nx-bail --userco... ✅ Succeeded 1m 51s View ↗
nx run-many --target=build ✅ Succeeded 18m 9s View ↗
nx affected --target=lint ✅ Succeeded 15m 25s View ↗
nx affected --target=test --collectCoverage ✅ Succeeded 10m 31s View ↗
nx affected --target=package-github-action ✅ Succeeded 2m 32s View ↗
Additional runs (3) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-01-22 08:47:52 UTC

@kpanot kpanot force-pushed the feature/analytics branch from 584122e to d570c4c Compare July 2, 2024 06:43
@kpanot kpanot force-pushed the feature/analytics branch from d570c4c to 7aaa320 Compare July 9, 2024 07:30
@kpanot kpanot marked this pull request as ready for review July 9, 2024 07:30
@kpanot kpanot force-pushed the feature/analytics branch from 7aaa320 to cbc141a Compare July 10, 2024 05:46
@kpanot kpanot force-pushed the feature/analytics branch 2 times, most recently from 83d6847 to c2d7f28 Compare August 13, 2024 06:05
@kpanot kpanot force-pushed the feature/analytics branch from c2d7f28 to d1ebcca Compare August 13, 2024 06:43
@kpanot kpanot force-pushed the feature/analytics branch 4 times, most recently from 843054d to 44f54ae Compare January 15, 2025 06:11
@codecov Codecov
Copy link

codecov bot commented Jan 15, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 56.60377% with 46 lines in your changes missing coverage. Please review.

Project coverage is 65.86%. Comparing base (b6b0ae6) to head (e4af13b).

✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ker/services/tracker/analytics-reporter.service.ts 15.78% 32 Missing ⚠️
...rty/google-analytics/google-analytics.analytics.ts 71.42% 5 Missing and 3 partials ⚠️
...r/analytics/src/tracker/logger/logger.analytics.ts 70.00% 6 Missing ⚠️
Additional details and impacted files

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kpanot kpanot force-pushed the feature/analytics branch 3 times, most recently from 055d1be to 3dae4b0 Compare January 15, 2025 09:37
cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/PERFORMANCE.md

The time to interactive is quite tricky as it not only depends on the relevant data readiness, but also on
component internal display mechanics.
If you know exactly where javascript will trigger a layout change (e.g. by passing a boolean variable to true), it's possible to measure the upper bound for the rendering.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a bit vague, maybe an implemented example could help the user understand better here

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/PERFORMANCE.md
If you know exactly where javascript will trigger a layout change (e.g. by passing a boolean variable to true), it's possible to measure the upper bound for the rendering.

In addition, during a component development, you can't possibly know beforehand if the component will be relevant for a TTI or not, since it depends on the page itself.
For example, the display of a cart component may be relevant for TTI in a given page and not relevant at all in others.
Copy link
Contributor

Choose a reason for hiding this comment

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

(product selection vs product recap?)

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/PERFORMANCE.md
As the browser can't understand when a route event happens in an SPA, the NavigationTimingAPI can't be directly used apart from the first page load at most.
Subsequent routing changes won't profit of the API connection timings.

In regard of the __server fetches__ (filter out from the resource timing API), the [PerformanceMetricPlugin](https://github.com/AmadeusITGroup/otter/blob/main/packages/@ama-sdk/core/src/plugins/perf-metric/perf-metric.fetch.ts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In regard of the __server fetches__ (filter out from the resource timing API), the [PerformanceMetricPlugin](https://github.com/AmadeusITGroup/otter/blob/main/packages/@ama-sdk/core/src/plugins/perf-metric/perf-metric.fetch.ts)
In regard of the __server fetches__ (filtered out from the resource timing API), the [PerformanceMetricPlugin](https://github.com/AmadeusITGroup/otter/blob/main/packages/@ama-sdk/core/src/plugins/perf-metric/perf-metric.fetch.ts)

@sdo-1A : could you confirm this one please?

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md
...

somethingHappened() {
this.eventTrackService.addUiEvent(new analyticsEvents.dummyEvent())
Copy link
Contributor

Choose a reason for hiding this comment

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

What does dummyEvent look like?

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md

## TrackEventsModule

The `TrackEventsModule` contains directives to help you track standard event such as the `TrackClickDirective` or
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `TrackEventsModule` contains directives to help you track standard event such as the `TrackClickDirective` or
The `TrackEventsModule` contains directives to help you track standard events such as the `TrackClickDirective` or

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md

The `TrackEventsModule` contains directives to help you track standard event such as the `TrackClickDirective` or
`TrackFocusDirective`.
You can track more standard ui event with the `TrackEventsDirective` and even create your own component events
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can track more standard ui event with the `TrackEventsDirective` and even create your own component events
You can track more standard ui events with the `TrackEventsDirective` and even create your own component events

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md
<button
(click)="doSomethingElse()"
trackClick
[trackEventContext]="analyticsEvents.runtimeDummyEvent('You could send runtime data here')"></button>
Copy link
Contributor

Choose a reason for hiding this comment

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

you should probably explain how these events look like before using them in the documentation

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md

### TrackEvents directive

The directive will listen to the events on the element on which was applied and will expose the event captured using the track service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The directive will listen to the events on the element on which was applied and will expose the event captured using the track service.
The directive will listen to the events on the element on which it was applied to, and will expose the event captured using the track service.

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md
| trackEvents | List of events which have to be tracked | ['mouseover', 'mouseenter'] |
| trackEventContext | Custom object to be exposed when the event is captured | {context: 'continueBtnClicked'} |

A specific directive for the click event was created, as it is the most used tracked event.
Copy link
Contributor

@cpaulve-1A cpaulve-1A Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
A specific directive for the click event was created, as it is the most used tracked event.
As the click is the most tracked event, the `@o3r/analytics` provides a dedicated directive to support it.

cpaulve-1A
cpaulve-1A reviewed Jan 20, 2025
View reviewed changes
docs/analytics/TRACK_EVENTS(deprecated).md

### Application level

At application level a subscription can be done to the observable emitted by the track events service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
At application level a subscription can be done to the observable emitted by the track events service.
At application level, the track events service can subscribe to observables emitted by tracking events.

@kpanot kpanot force-pushed the feature/analytics branch from 3dae4b0 to 4ede4dd Compare January 22, 2025 06:56
@kpanot kpanot force-pushed the feature/analytics branch from 4ede4dd to e4af13b Compare January 22, 2025 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpaulve-1A cpaulve-1A cpaulve-1A left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
enhancement New feature or request project:@ama-sdk/schematics project:@o3r/analytics project:@o3r/core project:@o3r/eslint-config project:@o3r/eslint-config-otter project:@o3r/logger project:@o3r/showcase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kpanot @cpaulve-1A