Add ProviderName and ProviderGuid properties to TraceTelemetry #120
Conversation
…ventSourceListener
|
@karolz-ms does this change looks good for you? |
| @@ -32,6 +32,9 @@ public static class EventDataExtensions | |||
| SeverityLevel.Verbose // EventLevel.Verbose == 5 | |||
| }; | |||
|
|
|||
| private static readonly string ProviderNameProperty = "ProviderName"; | |||
There was a problem hiding this comment.
Consider const instead of static readonly for these two strings.
| @@ -61,7 +64,9 @@ public static TraceTelemetry CreateTraceTelementry(this EventWrittenEventArgs ev | |||
| /// <param name="telemetry">Telemetry item to populate with properties.</param> | |||
| /// <param name="eventSourceEvent">Event to extract values from.</param> | |||
| public static TraceTelemetry PopulateStandardProperties(this TraceTelemetry telemetry, EventWrittenEventArgs eventSourceEvent) | |||
| { | |||
| { | |||
| telemetry.AddProperty(ProviderNameProperty, eventSourceEvent.EventSource.Name); | |||
There was a problem hiding this comment.
Check if the EventSource.Name is not null and only add the property if it isn't
| { | ||
| { | ||
| telemetry.AddProperty(ProviderNameProperty, eventSourceEvent.EventSource.Name); | ||
| telemetry.AddProperty(ProviderGuidProperty, eventSourceEvent.EventSource.Guid.ToString()); |
There was a problem hiding this comment.
Check if the Guid is different from default(Guid) and only add the property if it isn't
There was a problem hiding this comment.
@karolz-ms, regarding these two changes, do you not feel a constant schema (i.e. always containing the same properties regardless of content) is preferable here? I'll implement these changes if you have a strong preference for them but I'd prefer a constant same schema and letting the consumer filter/ignore irrelevant data (sometimes empty data is actually information in itself).
Obviously your call though.
There was a problem hiding this comment.
@ibebbs thanks for a good question
About the Guid property I feel pretty strongly. We do this check in other places, and, more importantly, an empty Guid, when serialized, will look like a legitimate Guid, albeit with a lot of zeros. This is confusing, IMO, and wastes space in your log data store.
The name property is less of a clear cut, but I still would prefer that we do not send it if it is not set (which, in case of EventSource, should be rare). I am not too concerned about variable schema--log data stores have to deal with variable schemas all the time, with respect to how the data is queried and how it is displayed. I am more concerned about how various outputs will serialize the null value--this has proven to be problematic in the past. Also, in general it is good to be able to differentiate between "not set" and "the value is empty" cases, although, arguably, for EventSource Name is not that big of a difference in practice.
There was a problem hiding this comment.
Hi @karolz-ms, thanks for taking the time to explain your thoughts. I completely see your point of view regarding the serialization of nulls and - as you rightly point out - this probably introduces more potential complexity/uncertainty than a variable schema.
I'll make the changes, add a couple of tests, and amend the PR.
|
@karolz-ms, I've implemented the suggested changes but haven't added any additional unit tests because, as far as I know, it's not possible to create an EventSource with a null/empty name and/or guid. Trust the changes are as per your intention. |
|
@ibebbs looks good, thanks for the update! |
|
@SergeyKanzhelev @cijothomas what is the ETA to have a new version of the EventSourceListener package published? |
|
@karolz-ms We are about to release 2.5.0-beta1 of base SDK. Logging adapters are typically published only along stable base SDKs, so in this case 2.5.0 which is 1-2 months away. |
No description provided.