Reduce coupling to TelemetryLogger #16464
Conversation
github-actions
bot
added
area: dds
| @@ -238,14 +260,13 @@ export abstract class TelemetryLogger implements ITelemetryLoggerExt { | |||
| const value = | |||
| typeof getterOrValue === "function" ? getterOrValue() : getterOrValue; | |||
| if (value !== undefined) { | |||
| // @ts-expect-error what | |||
There was a problem hiding this comment.
this was a leftover artifact of the previous PR, #16385, this is just a bit of clean up to remove the expected error, which was spurious, just had to find typing to work around it.
⯅ @fluid-example/bundle-size-tests: +7.71 KB
Baseline commit: 65c8073 |
| private extendProperties<T extends ITelemetryLoggerPropertyBag = ITelemetryLoggerPropertyBag>( | ||
| toExtend: T, |
There was a problem hiding this comment.
Still a bit confused here. The object we're passing as first parameter is typed as ITelemetryBaseEvent (external interface, from core-interfaces) which extends ITelemetryProperties, a "subset" of the internal ITelemetryLoggerPropertyBag (the latter supports getters as values). The function doesn't check the passed toExtend for getter-values, only TelemetryLogger's own properties. Would this be more appropriate?
| private extendProperties<T extends ITelemetryLoggerPropertyBag = ITelemetryLoggerPropertyBag>( | |
| toExtend: T, | |
| private extendProperties<T extends ITelemetryProperties = ITelemetryProperties>( | |
| toExtend: T, |
There was a problem hiding this comment.
i didn't really change this code, i just moved it, it actually wasn't even necessary to move, it was an artifact of some other changes i was porting over from a different branch. if you look at prepare event, which is the only place this is called, and where it used to live, that code takes ITelemetryBaseEvent which extents ITelemetryLoggerPropertyBag, so i think this change is correct.
I agree the typing is messed up here. It actually looks like a problem with ITelemetryPropertiesExt which probably should support getters, @markfields who should probably look into that.
There was a problem hiding this comment.
i don't actually get ITelemetryPropertiesExt, or how that works with general object vs tagged objects, seems complicated, i'm also not sure i see the benefit to logging arrays and objects.
There was a problem hiding this comment.
Yeah the property bag types don't have Ext verions. See https://dev.azure.com/fluidframework/internal/_queries/edit/4552, it's a leftover/low-pri todo from the work to add array support.
Tony, happy to discuss the "why" of supporting arrays and objects elsewhere
There was a problem hiding this comment.
@anthony-murphy why isn't it just this? There's only one caller and it passes an ITelemetryBaseEvent.
| private extendProperties<T extends ITelemetryLoggerPropertyBag = ITelemetryLoggerPropertyBag>( | |
| toExtend: T, | |
| private extendProperties(eventLike: ITelemetryBaseEvent, |
There was a problem hiding this comment.
its because i shouldn't have originally include it. i want to add a function to pull the properties off a logger to add to errors, which will reuse this, and has really been my end goal all along :)
I'm deprecating TelemetryLogger, which in turn will make MockLogger a ITelemetryBaseLogger. These places all have unit tests that use MockLogger, so will break. To get around this we can wrap the MockLogger in a child logger to make it a ITelemetryLoggerExt, but in these cases that is also unnecessary as the cases already create a child logger with the passed in logger, so downgrading the type they take to ITelemetryBaseLogger will make the change to MockLogger transparent. Overall, i think we should move away from passing around ITelemetryLoggerExt. I actually think we should make it internal. We will then pass around ITelemetryBaseLogger and create child loggers where necessary, and maybe add some internal helper functions for the few case were we pass ITelemetryLoggerExt into a func, only to call a single method on it, like sendError. Overall, this would make things much more consistent, e.g. always use ITelemetryBaseLogger, which also makes testing easier, as the interface is simpler. I'm trying to get it such that we have a single pattern for doing things, not multiple that are kind of the same, but have nuances why they are different, as i think it makes it harder for us to get right, which can lead to inconsistency for consumers. |
| @@ -56,7 +56,7 @@ const defaultNumberSummarizationAttempts = 2; // only up to 2 attempts | |||
| */ | |||
| export class RunningSummarizer implements IDisposable { | |||
| public static async start( | |||
| logger: ITelemetryLoggerExt, | |||
There was a problem hiding this comment.
Feels like within the runtime we should be fine to pass around ITelemetryLoggerExt, why the change?
There was a problem hiding this comment.
I found the existing convo, got it.
Can you write up a one-pager and post on Dev and/or bring it to Design Review? Seems reasonable but would be good to discuss even briefly to check pros/cons |
is there any documentation for how the current types work? it seems like a bit of a mess. my proposal is make it not a mess. |
|
@anthony-murphy let's revisit the "mess" after that ADO item about removing the old/duplicate types is done. |
In a previous PR I deprecated the telemetry logger implementation. This change further decouple existing code from the TelemetryLogger, including a future change that will modify MockLogger to no longer inherit from TelemetryLogger, and only implement ITelemetryBaseLogger.