ITelemetryBaseLogger optional shutdown()

[jvoels]

FluidFramework/common/lib/common-definitions/src/logger.ts

Line 33 in cd3c8a8

export interface ITelemetryBaseLogger {

Raising the discussion if ITelemetryBaseLogger should expose an optional shutdown method for the need to cease event flow, flush, and release resources as soon as deemed by any point in the application flow where this interface typically represents the contract for the logger.

The inspiring use case I have currently is when a hosting application that is running against a non-test, non-local environment is equipped with test in production hooks and used to side load experimental code changes. This hosting application is still generating telemetry data that is used in a non-test, non-local store with dimensions that had been and still may continue to imply critical monitoring and metrics signals. They are effectively invalidated by the side-loading activities.

Since the telemetry data is used in N > 1 consumption patterns by multiple systems, a DRY solution is needed upstream near or in front of the event collection pipelines to exclude potentially misleading or invalid event streams. The most obvious and earliest way to achieve this seems to be to allow the client application/host to stop sending events in such scenarios.

Motivations may vary but this does not seem to be an entirely unique concept for a logger. It shows up in public documentation for MSBuild logger and open telemetry specification.

@hedasilv @TriDucNgo @znewton

[Zlatkovsky]

Note: if we do add a shutdown() method like @jvoels is proposing, which I am in full support of, I would recommend adding it as an optional method on the interface. Even from the standpoint of managing back-compat, having it optional would make things easier (and that way it's not even a breaking change), plus it gives Logger providers the option to not implement the method. But having it as an optional on the interface none-the-less will give us the ability to have code that does:

logger.shutdown?.();

And have it no-op if no shutdown method is provided, which I think is the best-of-both-worlds.

[jvoels]

There's benefits on my end for adding it in either fashion :)

We should solve first for whether or not it belongs, then follow that with how it belongs.

As a general pattern optional methods definitely have benefits for non-breaking changes, but as these accumulate the API owners would want to be cognizant of when the break should just be made or a new interface introduced and moved toward.

One question for planning out the API across versions that can take breaks is really: If we were for some reason building the interface today with no back compat limitations, would we actually choose to do it this fashion or not?

[hedasilv]

Adding @markfields who was also looking into client telemetry

[markfields]

@jvoels - I think there are two use cases we're discussing here:

1. Analog to IDisposable.dispose, for releasing resources, flushing buffers, etc. Glancing at the two "prior art" examples, I think this is what they're about. I also had to add this to our logging interface when running test automation.
2. We need to avoid poisoning clean production telemetry with data from TIP/experimental configurations.

For (1), I'm not sure it belongs on ITelemetryBaseLogger. This interface is for the host application to provide a way for the FF internals to log. The host itself holds the logger by a different abstraction which probably exposes a shutdown/flush/dispose capability, which the host will call as it shuts down.

For (2), we have ways to add common metadata to all logs within a given context, so could we add a property to the logs that we filter on when we want pure production telemetry? That way we still get telemetry in the experimental cases, which could be valuable. And as we roll out richer feature flag support, the lines could get blurry here.

So I'm not convinced yet :P Thoughts?

[jvoels]

I think you've for now nailed it with the separation of concerns between participating in logging (send) and managing logging (shutdown)

And I think this potential panacea arises primarily from signs of the problem in #2 which should be remedied in the design of features that corrupt the data - they must not corrupt it to begin with. Each logging system will have it's own strategy here that ideally doesn't benefit from this idea anyway.