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

chore(feature): knowledge hub Telemetry Setup #68

Open
wants to merge 51 commits into
base: main
Choose a base branch
from

Conversation

aakankshagupta1994
Copy link

@aakankshagupta1994 aakankshagupta1994 commented Apr 17, 2023

Issue

#35
#36

Description

TBI - Provide telemetry data for the tutorials and tutorial tags opened
TBI - Provide telemetry data for the blogs and tags opened

FILL IN DETAILS HERE

Checklist for Pull Requests

  • Supplied as many details as possible on this change
  • Included the link to the associated issue in the Issue section above
  • The code conforms to the general development guidelines.
  • The code has unit tests where applicable and is easily unit-testable
  • This branch is appropriately named for the associated issue

@aakankshagupta1994 aakankshagupta1994 requested a review from a team as a code owner April 17, 2023 23:04
@changeset-bot
Copy link

changeset-bot bot commented Apr 17, 2023

⚠️ No Changeset found

Latest commit: f373855

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@aakankshagupta1994 aakankshagupta1994 changed the title Chore/renovate/telemetry Chore(feature): Knowledge Hub Telemetry Setup Apr 18, 2023
@aakankshagupta1994 aakankshagupta1994 changed the title Chore(feature): Knowledge Hub Telemetry Setup chore(feature): Knowledge Hub Telemetry Setup Apr 18, 2023
@aakankshagupta1994 aakankshagupta1994 changed the title chore(feature): Knowledge Hub Telemetry Setup chore(feature): knowledge hub Telemetry Setup Apr 18, 2023
Copy link
Contributor

@kranthie-sap kranthie-sap left a comment

Choose a reason for hiding this comment

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

Hi @aakankshagupta1994, please see my review comments. Also, please update the ide-extension telemetry unit tests to use the right telemetry events send from the webapp. Also, please add unit tests in the webapp to simulate clicks and check if the action is triggered.

packages/ide-extension/package.json Outdated Show resolved Hide resolved
packages/ide-extension/package.nls.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
packages/ide-extension/src/panels/knowledgeHubPanel.ts Outdated Show resolved Hide resolved
packages/ide-extension/src/telemetry/telemetry.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
packages/webapp/src/webview/store/actions.ts Outdated Show resolved Hide resolved
packages/webapp/src/webview/store/actions.ts Outdated Show resolved Hide resolved
packages/webapp/src/webview/store/actions.ts Outdated Show resolved Hide resolved
packages/webapp/src/webview/store/actions.ts Outdated Show resolved Hide resolved
@kranthie-sap
Copy link
Contributor

@aakankshagupta1994
Copy link
Author

@kranthie-sap The requested changes are added. Can you please verify?

Copy link
Contributor

@Klaus-Keller Klaus-Keller left a comment

Choose a reason for hiding this comment

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

@kranthie-sap @aakankshagupta1994
the two tracked telemetry events in this pull request are for links that are clicked and need a new redux action, so far so good. However, it is often the case that an existing redux action can be used as trigger to track telemetry data. In this case, to avoid polluting application logic with telemetry events an alternative is to add a telemetry middleware and filter/map redux actions that should be tracked.
We can discuss this along with this PR or postpone the discussion until more events are added, whatever you prefer.

packages/ide-extension/src/telemetry/types.ts Outdated Show resolved Hide resolved
packages/ide-extension/src/telemetry/types.ts Outdated Show resolved Hide resolved
packages/webapp/src/webview/store/middleware.ts Outdated Show resolved Hide resolved
packages/types/src/types/actions.ts Outdated Show resolved Hide resolved
packages/ide-extension/src/telemetry/types.ts Show resolved Hide resolved
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

82.0% 82.0% Coverage
0.0% 0.0% Duplication

Copy link
Contributor

@Klaus-Keller Klaus-Keller left a comment

Choose a reason for hiding this comment

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

Hi @aakankshagupta1994,

Thanks for addressing all my comments. There are no open issues that I can see, from my side this is approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants