Extensible payload handler #111
Conversation
|
@jafletch, It will cover your contributions to all Microsoft-managed open source projects. |
|
@jafletch, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
@jafletch, it looks similar to the existing concept of Telemetry Initializers that allow one to modify/enrich telemetry item context before it gets sent. Is there another scenario or requirement this change might address? |
|
@Dmitry-Matveev I am pretty new to AI in general and I just glanced through the docs in initializers and processors and it isn't obvious that would solve my problem, but perhaps I am just not familiar enough. The issue that I have is that I need to get access to the underlying ETW EventData object before that gets encoded into the properties of the TraceTelemetry object. The current implementation very simply passes over that EventData calling ToString() on the various parts of the payload and puts those in as properties of TraceTelemetry. I like most of what the current implementation does, but I want to be able to override that one behavior. I am open to any option that lets me do that. |
|
@jafletch how about we give you an event off of the module, where you would get the EventData object and the TraceTelemetry object produced out of it and you could massage the TraceTelemetry object the way you see fit before it is passed down the pipeline? |
|
@karolz-ms Sure, that sounds like basically what my change enables. For reference, here is my OnEventWrittenHandler currently looks like: public static void Track(EventWrittenEventArgs eventData, TelemetryClient client)
{
var telemetry = eventData.ToTraceTelementry(includePayload: false);
eventData.ExtractPayloadData(telemetry);
client.Track(telemetry);
}where the ToTraceTelementry extension method is just a slightly altered version of the Track EventData extension method that previously existed in the code and the ExtratPayloadData method is my own custom code. So I am able to reuse all of the functionality of setting up the TraceTelemetry object that this code was previously doing and just opt out of the payload handling part. If you passed me the TraceTelemetry object with the payload fields already in there per the current ToString() mechanism, that doesn't seem useful so I would want to make sure that I get full control over handling the payload. |
|
@jafletch Two questions then
if the answer is "no"
The latter is to ensure (to the foreseeable extent) that if someone else comes along and wants to create |
|
@karolz-ms No, I wouldn't consider my solution sufficient for all cases. We are lucky in that we have a pretty narrow set of things that come through payload and my implementation is enough to handle those, but it isn't something I would be comfortable shipping much more broadly. I think this is going to be a pretty hard problem for a general solution which is why I think this particular extensibility point will be essential. Agreed on point (2). I'll make some changes there and push them up. Thanks! We already have people waiting on AI support so this is pretty cool for us. |
|
I wonder why do we need an extensibility on telemetry module when it may be more natural to implement a custom eventsource listener? The benefit of custom listener is that you get a full control over it's behavior and granularity of configuring it only for the event sources you are interested in. |
|
@SergeyKanzhelev the granularity part is also present with our |
|
Ok I believe the current code reflects the requested changes |
|
@karolz-ms my question was what benefit telemetry module gives comparing to the custom event listener. It feel like close to zero code reuse unless I missing something important. |
…ights-dotnet-logging into extensible-payload-handler # Conflicts: # src/Adapters/EventSource.Netstandard13/EventSourceTelemetryModule.cs
…tch/ApplicationInsights-dotnet-logging into extensible-payload-handler
|
To give some context here - exposing a callback to allow one to extract more data is a very common scenario. Same way you need access to So long term we need a new layer in the SDK layering model that will enable such scenario in a more generic way. We discussed it with @karolz-ms offline at Friday and floated ideas like I think we need to get this change and later refactor it into the generic approach. I'd also like to know what would be your experience with the callback approach and ideas around it. |
| @@ -106,7 +120,7 @@ protected override void OnEventWritten(EventWrittenEventArgs eventData) | |||
| // and not that useful for production tracing. However, TPL EventSource must be enabled to get hierarchical activity IDs. | |||
| if (this.initialized && !TplActivities.TplEventSourceGuid.Equals(eventData.EventSource.Guid)) | |||
| { | |||
| eventData.Track(this.client); | |||
| this.onEventWrittenHandler(eventData, this.client); | |||
There was a problem hiding this comment.
put try catch around it as it is an external code you are calling
| @@ -15,7 +15,7 @@ namespace Microsoft.ApplicationInsights.EventSourceListener.Implementation | |||
| using Microsoft.ApplicationInsights.DataContracts; | |||
| using Microsoft.ApplicationInsights.TraceEvent.Shared.Utilities; | |||
|
|
|||
| internal static class EventDataExtensions | |||
| public static class EventDataExtensions | |||
There was a problem hiding this comment.
Why making it public? Do you want to re-use certain methods from here?
There was a problem hiding this comment.
Yes, the PopulateStandardProperties method is very useful for outside creators who want to write their own payload readers but make still use of the rest of the properties.
There was a problem hiding this comment.
Perhaps the callback may be called after telemetry got initialized then? My worry is that if you use this method you probably relying on the implementation too much. It feel you want a callback to "add" information, not to replace. Is it correct?
There was a problem hiding this comment.
Perhaps I am not the standard consumer here but it works exactly as I would expect it to work and it handles the job of that I would just end up implementing myself if it wasn't there. For my scenario, I call:
var telemetry = eventData.ToTraceTelementry()
.PopulateStandardProperties(eventData);
// My custom Payload handler
eventData.ExtractPayloadData(telemetry);
This gives me a TraceTelemetry with all of the "boilerplate" fields populated from the EventWrittenEventArgs and then I add my own payload onto that. Of course you need not call any of these methods if you don't want to, but I find them convenient.
There was a problem hiding this comment.
thank you! I appreciate you taking time to submit this PR and have this conversation. It definitely helps to form the opinion on how analogous generic API should look like.
|
Ok I believe all the feedback is addressed now |
We use Tracelogging extensively in our system and we often log nested EventData objects via Write. The current implementation doesn't work for this scenario since it just calls ToString() on each value in the Payload collection. This change allows for implementing your own handler for converting EventWrittenEventArgs to TraceTelemetry.