Support for flat arrays in TelemetryLogger

[daesun-park]

Description

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. Accept a less-restrictive type than TelemetryEventPropertyType which includes arrays or objects where the values are strictly primitives (TelemetryEventPropertyType).
2. Stringify these flat objects/arrays when passing to another logger.

Reviewer Guidance

Questions

1. As mentioned by @markfields in the original internal task 1104, updating TelemetryEventPropertyType should maybe be avoided, as it would be quite a breaking change. Instead, I defined the following types/interfaces within the logger.ts file where our TelemetryLogger is implemented (instead of @fluidframework/common-definitions).

• TelemetryEventPropertyTypeExt
• ITelemetryBaseEventExt
• ITelemetryGenericEventExt
• ITelemetryPerformanceEventExt;

However, would it be best to instead change the update the existing types/interfaces defined in the @fluidframework/common-definitions instead?

2. As this pull request aims to provide support for flat arrays, which test cases should be updated/added to both the end-to-end and unit tests?

Link to Internal Task #1104

[msfluid-bot]

⯅ @fluid-example/bundle-size-tests: +2.79 KB

Metric Name	Baseline Size	Compare Size	Size Diff
aqueduct.js	387.57 KB	387.9 KB	⯅ +345 Bytes
connectionState.js	680 Bytes	680 Bytes	■ No change
containerRuntime.js	191.68 KB	192.02 KB	⯅ +345 Bytes
loader.js	151.53 KB	151.95 KB	⯅ +435 Bytes
map.js	42.67 KB	43.01 KB	⯅ +344 Bytes
matrix.js	127.15 KB	127.49 KB	⯅ +344 Bytes
odspDriver.js	149.41 KB	149.75 KB	⯅ +346 Bytes
odspPrefetchSnapshot.js	38.31 KB	38.65 KB	⯅ +352 Bytes
sharedString.js	147.79 KB	148.13 KB	⯅ +345 Bytes
Total Size	1.24 MB	1.24 MB	⯅ +2.79 KB

---

Baseline commit: 8422abd

Generated by 🚫 dangerJS against d1e714a

[vladsud]

I see 3 solutions here:

1. We change core types and push responsibility to serialize arrays to consumers (hosts). This is breaking change as it impacts ITelemetryBaseLogger() and host implementation.
   • pros: much simpler types / handling, native support of arrays.
   • con: needs to be staged properly, pushes some complexity to hosts.
2. What you are doing
3. Same as above, but define unique types, not use unions. I.e. sendTelemetryEvent(event: ITelemetryGenericEventExt)

I prefer # 3 over # 2 - it makes it much easier to read code / follow types.
I'd say that it's worth going further - all of our code should use these types! So, either you follow up to replace all the usage in repo (where ITelemetryLogger interface is used, which is pretty much 99% of our code base) to these new types, or we reuse existing types to extend them, and create "legacy" (or rather - upstream types) for ITelemetryBaseLogger to be unchanged (has some implication on hosts).
Basically, we quite often propagate telemetry types through stack, and it would be pain for engineers to incrementally make these changes - I'd rather see this change done in one go.

I prefer # 1 over # 3. :) While it will take time to stage, I think change and final state will be much better.

[daesun-park]

Sounds great Vlad, I will revert the changes and approach it with solution 1 :)

[vladsud]

Please talk to Bohemia team RE this, as we would need some staging.
If there is pushback, what we can do also (not ideal, but workable solution) - is modified # 3, i.e. we create new set of interfaces that are to be used by ITelemetryBaseLogger and are copies of what we had before (such that host, for most part, does not need to do anything, other than using new names of interfaces), and reuse existing names to include arrays. TelemetryLogger can always json arrays to bridge the gap.

Please note that if you go on # 1, you need to be aware that staging (waiting) will be long. That's because version N of runtime may start to use arrays, and it maybe the version that app uses, but previous version of an app (like Chapter 1 app) does not have handling for arrays, but loads dynamically "future" version of container runtime and driver (that are on version N), which produce telemetry with arrays. That may break things.
So first, we need to ensure that all consumers can handle arrays. Wait some substantial amount of time for these changes to get to 100% prod saturation, and only then allow us to leverage arrays.
There are multiple ways to approach this problem. The one I'd suggest is the following:

• all the types are changed to support arrays, and we make a note in breaking.md that all apps have to deal with array
• but for some long period of time (multiple internal major releases), TelemetryLogger continuous to JSON arrays.
• and only after some reasonable amount of time, we remove that code and let hosting app layer to deal with arrays.

[vladsud]

Might want to support null as well here, given you are making these changes. We actually pass null all the time (ISequencedDocumentMessage.clientId can be null, despite type saying otherwise - just opened ticket to fix it).

You probably want to allow undefined to be element of array either.
Though this is slippery, as it will show up as null after stringify / parse cycle.

[vladsud]

@daesun-park, is the description of the PR updated based on recent changes?
Did I get it right that what you are trying to do is

• Allow new types on ChildLogger
• Do not change types for base logger.
• Address the conversion through TaggedLoggerAdapter

Is that right?
That's reasonable directly, but as I pointed out earlier, it will require changes in most of our code. Search for ITelemetryProperties. Most of the usage in repo would need to be updated to ITelemetryPropertiesExt.

As a general direction, I think I like for type system we use internally to be different from type system used by base logger - it allows us to support more types (and convert them to strings or similar) without putting more requirements on the hosts.
But I'd rather see it reflected in all of our repo (likely as two PRs).

While it's technically "breaking" change, I'd rather go with ITelemetryBaseProperties & ITelemetryProperties pair vs. ITelemetryProperties & ITelemetryPropertiesExt pair.

[ghost]

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!