Skip to content

Quick Pulse cleanup and add sample telemetry feature#1852

Merged
trask merged 26 commits into
mainfrom
kryalama/quickpulsecleanup
Aug 30, 2021
Merged

Quick Pulse cleanup and add sample telemetry feature#1852
trask merged 26 commits into
mainfrom
kryalama/quickpulsecleanup

Conversation

@kryalama

@kryalama kryalama commented Aug 14, 2021

Copy link
Copy Markdown
Contributor

Quick Pulse cleanup
Add playback tests
Add Sample Telemetry feature to live metrics

@kryalama kryalama marked this pull request as ready for review August 18, 2021 18:01

@trask trask left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

can you set up a meeting to walk me through the playback tests?

@kryalama kryalama changed the title Quick Pulse cleanup Quick Pulse cleanup and add sample telemetry feature Aug 25, 2021
@kryalama kryalama requested a review from trask August 25, 2021 17:50
}
}

private static Map<String, String> aggregateProperties(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

C# SDK limits the maximum number of Properties and Measurements to 10. But if you feel this is not necessary here, I won't insist on it. Trask will probably have an opinion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If you do choose to limit the top count, make sure it's consistent, so sort them by name first, and then limit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@trask your thoughts on this? Nodejs sdk donot have this limit

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's ok as-is for now, property/measurement explosion is not currently one of my worries, and we can revisit later

@kryalama kryalama requested a review from trask August 27, 2021 19:22

@trask trask left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🥳

@trask trask merged commit 532c7fe into main Aug 30, 2021
@trask trask deleted the kryalama/quickpulsecleanup branch August 30, 2021 20:50
@kryalama kryalama added this to the 3.2.0-BETA.3 milestone Sep 3, 2021
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