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

Add possibility to send telemetry events by data collectors #4622

Merged
merged 13 commits into from
Jul 31, 2023

Conversation

jakubch1
Copy link
Member

Description

Please add a meaningful description for this change.
Ensure the PR has required unit tests.

Related issue

Kindly link any related issues. E.g. Fixes #xyz.

  • I have ensured that there is a previously discussed and approved issue.

@jakubch1 jakubch1 changed the title [draft] telemetry 2 Add possibility to send telemetry events by data collectors Jul 28, 2023
@jakubch1 jakubch1 marked this pull request as ready for review July 28, 2023 13:07
Copy link
Member

@cvpoienaru cvpoienaru left a comment

Choose a reason for hiding this comment

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

LGTM

/// <summary>
/// Initializes telemetry reporter
/// </summary>
void Initialize(ITelemetryReporter telemetryReporter);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should have an option object for future extensibility but maybe we can add to the ITelemetryReporter self if needed

Copy link
Member Author

Choose a reason for hiding this comment

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

I think extending API for extensions is not issue anymore.

/// </summary>
/// <param name="name">Telemetry event name</param>
/// <param name="properties">Telemetry event properties</param>
public TelemetryEvent(string name, IDictionary<string, object> properties)
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit generic but underneath we don't have more than that

Copy link
Member Author

Choose a reason for hiding this comment

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

this is API used by VS telemetry

Copy link
Contributor

@MarcoRossignoli MarcoRossignoli left a comment

Choose a reason for hiding this comment

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

some comment but I don't know if we can do better

@jakubch1 jakubch1 merged commit f60b983 into microsoft:main Jul 31, 2023
7 checks passed
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