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

Use createTelemetryLogger api #11

Merged
merged 1 commit into from
Jul 7, 2023
Merged

Conversation

jeanp413
Copy link
Member

@jeanp413 jeanp413 commented Jun 2, 2023

Description

Use createTelemetryLogger api

How to test

  • in preview env using updated gitpod-web image tag

@jeanp413 jeanp413 requested a review from mustard-mh July 5, 2023 19:35
@jeanp413 jeanp413 marked this pull request as ready for review July 5, 2023 19:35
const idx = eventName.indexOf('/');
eventName = eventName.substring(idx + 1);

const properties = data ?? {};
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
const properties = data ?? {};
const properties = mixin(data ?? {}, commonProperties);

common properties are missing

Copy link
Contributor

Choose a reason for hiding this comment

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

see
image

Copy link
Member Author

Choose a reason for hiding this comment

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

they should be already there as they are added internally by vscode , that's should only needed for sendErrorData 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But you see the screenshot, it fixed (version / name are correct) after I mix the common properties

Copy link
Contributor

@mustard-mh mustard-mh Jul 6, 2023

Choose a reason for hiding this comment

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

So the common properties are missing in additionalCommonProperties ? Or this is expected to not send properties like extversion? Or it won't work only with debug mode?

Copy link
Member Author

@jeanp413 jeanp413 Jul 6, 2023

Choose a reason for hiding this comment

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

ah wait you tested from sources? then that's expected, it's because vscode will do validation for files in extension folder and telemetryService is in a different folder gitpod-shared, we should test it using webpacked version, i.e. in preview env using updated gitpod-web image tag

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mustard-mh
Copy link
Contributor

mustard-mh commented Jul 7, 2023

Tested in preview env 👍
image

@mustard-mh mustard-mh self-requested a review July 7, 2023 01:12
@mustard-mh mustard-mh merged commit 9de665d into master Jul 7, 2023
2 checks passed
@jeanp413 jeanp413 deleted the jp/intermediate-eagle branch July 7, 2023 02:23
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.

2 participants