FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

Separate telemetry property type sets

Open daesun-park opened this issue 2 years ago • 1 comments

Description

This pull request is a continuation of #11147 When using the telemetry logger, there are specific instances where logging a flat array or JSON object to an event field is necessary. To accommodate for these scenarios, this pull request focuses on making the following changes to the implementation of the TelemetryLogger class.

  1. Separate the types used in our Telemetry system to use two different sets ITelemetryBaseProperties & ITelemetryProperties.
  2. We change the core types ITelemetryProperties to have native support for arrays.
  3. The ITelemetryBaseProperties will have the same properties from our original ITelemetryProperties set (prior to support for arrays), and the responsibility of stringifying the arrays will be pushed to the hosts. The hosts will also have to change their parameter types to use ITelemetryBaseProperties.
  4. Any send() method for an event type which is not a baseEvent, will call the stringify function on the event prior to sending.

Reviewer Guidance

  1. For now, I have separated the types into 2 sets ITelemetryBaseProperties & ITelemetryProperties, where only the ITelemetryBaseEvent extends the ITelemetryBaseProperties. Therefore, the ITelemetryPerformanceEvent, ITelemetryGenericEvent and ITelemetryErrorEvent events currently supports arrays. I currently have the stringify function in the sendTelemetryEvent, sendTelemetryEventCore, and sendErrorEvent methods in our TelemetryLogger class.

As the hosts will now have to change their type names to be aligned with the new set ITelemetryBaseProperties, which documentation be updated to notify them of the changes?

  1. I have also tried to add the null type to the ITelemetryProperties as it was suggested in one of the previous threads from #11147 . However, I seem to get a deprecation warning, suggesting to use undefined instead (which is already included). Should I still proceed with adding support for the null type as well?

daesun-park avatar Jul 28 '22 23:07 daesun-park

@fluid-example/bundle-size-tests: +3.56 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 390.02 KB 391.21 KB +1.19 KB
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 193.16 KB 193.08 KB -83 Bytes
loader.js 151.53 KB 151.88 KB +363 Bytes
map.js 42.67 KB 43 KB +334 Bytes
matrix.js 128.2 KB 127.79 KB -422 Bytes
odspDriver.js 149.45 KB 149.73 KB +288 Bytes
odspPrefetchSnapshot.js 38.31 KB 38.6 KB +293 Bytes
sharedString.js 148.77 KB 150.39 KB +1.62 KB
Total Size 1.25 MB 1.25 MB +3.56 KB

Baseline commit: d9ee2c414c03010dd2e6d3dacdb869854ce74210

Generated by :no_entry_sign: dangerJS against c77c0314b6172c868c97d62d00d7e3596a818c23

msfluid-bot avatar Aug 03 '22 15:08 msfluid-bot

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

ghost avatar Dec 05 '22 00:12 ghost