Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow customers to provide Telemetry properties at individual item level, and at global level. #820

Closed
cijothomas opened this issue May 30, 2018 · 17 comments
Assignees
Milestone

Comments

@cijothomas
Copy link
Contributor

cijothomas commented May 30, 2018

Present State
Currently ITelemetry has a TelemetryContext which can define the context of the ITelemetry item. This context holds information that are global than the individual item properties. Apart from well known fields like Cloud ("ai.cloud.roleinstnace"), Device("ai.user.devicetype") etc., TelemetryContext also contains a property bag collection to store any key,value (string,string) pair of information.

All concrete ITelemetry implementations defined in the base sdk (eg: RequestTelemetry, DependencyTelemetry) also implements ISupportProperties interface.
This gives them a 'Properties' bag which can store any key,value (string,string) pair of information applicable to the individual telemetry item.

TelemetryClient also contains a TelemetryContext and properties from it are automatically applied to all the telemetry items tracked using that instance of TelemetryClient.
In effect, there are 3 places to set properties about an item.

  1. Using Properties of ISupportProperties
  2. Using Properties of TelemetryContext of ITelemetry
  3. Using Properties of TelemetryContext of TelemetryClient (more like a helper method, as this property bag is simply copied to (2) in Track() method.

For the current json wire format, all the above properties are serialized into "properties" section inside the telemetry.

The present implementation internally uses same Dictionary to hold the properties of individual item, and that from TelemetryContext.
i.e
var req = RequestTelemetry()
req.Properties and req.Context.Properties are internally the same collection, each being affected by modification to the other.

Issues
Since all properties are serialized into same "properties" bag in json, AI back-end cannot distinguish whether any given property belongs to just individual item or it belongs to a more global context.
Withtout this information back-end or UI cannot make special provisions for the global properties.
For eg: UI could chose to index global properties for fast filtering.

As the current implementation internally uses the same object to hold both properties, users has no way of informing AI that some properties are meant to
be global and some are only applicable to telemetry item.

Proposal
We want to provide users with a way to specify properties for telemetry items in 2 levels - one is local to the individual item, and can be high cardinality.
Other is more of a 'global' property, which AI back-end could chose to treat special. (For eg: by providing UI filters for global propps)

By splitting the Properties of ISupportProperties and Properties of TelemetryContext into separate collection, user can achieve this.
But this might be breaking change for customers who counted on these two property bags being the same.

So we'd like to provide a new field in TelemetryContext, called GlobalProperties (making it obvious to users that these are global level properties, and to avoid confusion with Properties on ISupportProperties). Users wishing to supply global properties could use GlobalProperties on TelemetryContext to do so.
For properties limited to individual item, Properties on ISupportProperties could be used. The Properties on TelemetryContext would be marked obsolete.

Note: For initial implementation, the GlobalProperties will be serialized into same "properties" bag in wire format (only in wire format). This is temporary until back-ends are changed. Also there are few internal users who would be using this SDK with custom channel+serialization, to send data to different back-ends (which support global vs local). They can take advantage of this right away.

Eg:

var tc = new TelemetryClient();
var req = new RequestTelemetry()
// User can continue to use old way of populating properties as shown below.
// These properties however will not be distinguished by AI back-end.
req.Context.Properties.Add("ObsoleteProp", "Value");  // Still works but obsolete

// Setting properties like the following make it obvious that this property is applied to the Context, rather than individual item. Once backend changes are in, these items would be serialized separately
req.Context.GlobalProperties.Add("GlobalProp", "TrueValue");

// Following works as before. Distinct collection from the Context.GlobalProperties, but same collection as deprecated Context.Properties
req.Properties.Add("LoalPropKey", "Value"); 

Since no breaking change, we will ship this in 2.7 itself. And use 3.0 to remove the obsolete fields.

@cijothomas
Copy link
Contributor Author

@pharring @lmolkova Please do share your comments!

@pharring
Copy link
Member

pharring commented Jun 1, 2018

I think I see what you're trying to do. I support having two property bags on telemetry items. However, I think that the low-cardinality one (GlobalProperties) should be set via the TelemetryClient. It should otherwise be internal (non-public).
This allows for optimization of the GlobalProperties implementation: It should be an immutable snapshot of the properties in the TelemetryClient's context -- and that snapshot can be created very efficiently (just a reference; no deep copying) since the properties are, by definition, not expected to change often -- they're low cardinality, as you say. That technique also opens up further optimizations at serialization time (i.e. remember/cache the results of serialization so you don't have to re-serialize the GlobalProperties each time).

@cijothomas
Copy link
Contributor Author

An initial version is in. Will submit a followup PR to address Pauls comments.

@cijothomas
Copy link
Contributor Author

This is added for 2.8.0-beta1 to allow longer beta period. (originally it was supposed to be in 2.7-beta3, which allowed too little time to gather feedback)

@johnkwaters
Copy link

I have a telemetryinitializer that for each Api Controller request logs the device ID and the tenant ID (from claims): would those be considered global or not? And I I want to add them to ISupportProperties - where do I find that in this context?

using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Http;
using NorthStar.Abstractions.Classes;
using NorthStar.Logic.Helpers;

namespace NorthStar.API.Classes
{
    /// <summary>
    /// Add custom App Insights telemetry
    /// </summary>
    public class JsonTelemetryInitializer : ITelemetryInitializer
    {
        private readonly IHttpContextAccessor _httpContextAccessor;

        /// <summary>
        /// Constructor
        /// </summary>
        /// <param name="accessor">For accessing the http context</param>
        /// <param name="systemOptions">System options</param>
        public JsonTelemetryInitializer(IHttpContextAccessor accessor)
        {
            _httpContextAccessor = accessor;
        }

        /// <summary>
        /// Initialize the custom telemetry initializer
        /// </summary>
        /// <param name="telemetry">Telemetry</param>
        public void Initialize(ITelemetry telemetry)
        {
            if (_httpContextAccessor.HttpContext == null)
            {
                return;
            }

            if (_httpContextAccessor.HttpContext.User.Identity.IsAuthenticated)
            {               
                const string tenantId = "northstar_tenantid";
                if (!telemetry.Context.Properties.ContainsKey(tenantId))
                {
                    var user = _httpContextAccessor.HttpContext.User;
                    telemetry.Context.Properties[tenantId] =
                        ClaimsHelper.GetClaim<int>(user, TokenNames.TenantId).ToString();

                    var userId = ClaimsHelper.GetClaim<int>(user, TokenNames.UserId).ToString();

                    telemetry.Context.Properties["northstar_userid"] = userId;
                    var deviceId = ClaimsHelper.GetClaim<string>(user, TokenNames.DeviceId);
                    if (deviceId != null)
                    {
                        telemetry.Context.Properties["northstar_deviceid"] = deviceId;
                    }

                    telemetry.Context.User.Id = userId;

                    var sessionId = ClaimsHelper.GetClaim<string>(user, TokenNames.SessionId);
                    if (!string.IsNullOrEmpty(sessionId))
                    {
                        telemetry.Context.Session.Id = sessionId;
                    }
                }
            }
        }
    }
}

@johnkwaters
Copy link

Is this how I would rewrite it? Should I use SupportProperties or GlobalProperties for this kind of per request info?

using Microsoft.ApplicationInsights.Channel;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.ApplicationInsights.Extensibility;
using Microsoft.AspNetCore.Http;
using NorthStar.Abstractions.Classes;
using NorthStar.Logic.Helpers;

namespace NorthStar.API.Classes
{
    /// <summary>
    /// Add custom App Insights telemetry
    /// </summary>
    public class JsonTelemetryInitializer : ITelemetryInitializer
    {
        private readonly IHttpContextAccessor _httpContextAccessor;

        /// <summary>
        /// Constructor
        /// </summary>
        /// <param name="accessor">For accessing the http context</param>
        public JsonTelemetryInitializer(IHttpContextAccessor accessor)
        {
            _httpContextAccessor = accessor;
        }

        /// <summary>
        /// Initialize the custom telemetry initializer
        /// </summary>
        /// <param name="telemetry">Telemetry</param>
        public void Initialize(ITelemetry telemetry)
        {
            const string tenantId = "northstar_tenantid";

            if (_httpContextAccessor.HttpContext == null ||
                !(telemetry is RequestTelemetry rt) || 
                !_httpContextAccessor.HttpContext.User.Identity.IsAuthenticated ||
                rt.Properties.ContainsKey(tenantId))
            {
                return;
            }

            var user = _httpContextAccessor.HttpContext.User;
            rt.Properties[tenantId] =
                ClaimsHelper.GetClaim<int>(user, TokenNames.TenantId).ToString();

            var userId = ClaimsHelper.GetClaim<int>(user, TokenNames.UserId).ToString();

            rt.Properties["northstar_userid"] = userId;
            var deviceId = ClaimsHelper.GetClaim<string>(user, TokenNames.DeviceId);
            if (deviceId != null)
            {
                rt.Properties["northstar_deviceid"] = deviceId;
            }

            telemetry.Context.User.Id = userId;

            var sessionId = ClaimsHelper.GetClaim<string>(user, TokenNames.SessionId);
            if (!string.IsNullOrEmpty(sessionId))
            {
                telemetry.Context.Session.Id = sessionId;
            }
        }
    }
}

@nholling
Copy link

Its not entirely obvious what is meant by the term "Global". Does "Global" mean global to the Application. Or does "Global" mean global to the request? This should be made clear in the documentation. Global is too vague of a term.

@cijothomas
Copy link
Contributor Author

Global here means global for the application - something which wont change for every request.

Agree there are no official docs, but some discussion spread in github issues...

TimothyMothra pushed a commit that referenced this issue Oct 25, 2019
Develop to master 2.6.0-beta3
@sonic1981
Copy link

sonic1981 commented Nov 2, 2020

So telemetaryClient.Context.Properties now contains the following obsolete message:

Use GlobalProperties to set global level properties. For properties at item level, use ISupportProperties.Properties.

Which when you google brings you here.

The official docs also contain a similar blanket message. A similar issue raised here has just been blanket closed pointing at this issue.

Feel free to open an issue under the AI-dotnet repo for tracking purpose.

But it is really not very clear in any of the documentation (that I can find) how I should use these new properties. For example I want to add a property at request level. How do I do that?

@sonic1981
Copy link

After much digging and experimenting eventually this seem provided me the solution #1428 (comment)

so the answer to adding request telemetry is:

Activity.Current?.AddTag("key", "value");

@cijothomas
Copy link
Contributor Author

For example I want to add a property at request level. How do I do that?

RequestTelemetry implements ISupportProperties. So you can do requestTelemetry.Properties.Add(key,value). This can be done in a TelemetryInitializer.

@cijothomas
Copy link
Contributor Author

After much digging and experimenting eventually this seem provided me the solution #1428 (comment)

so the answer to adding request telemetry is:

Activity.Current?.AddTag("key", "value");

This'd be incorrect, as applicationinsights SDK by default does not copy Activity tags into any telemetry.

@cijothomas
Copy link
Contributor Author

Please use the example here: https://docs.microsoft.com/en-us/azure/azure-monitor/app/api-filtering-sampling#addmodify-properties-itelemetryinitializer which shows how an initializer is used to add additional properties to the Request

@sonic1981
Copy link

I don't have the data at the TelemetryInitializer stage. My data is calculated and i want to tag the request with this data. I'm interested as to why you think the Activity. Is incorrect. I've tested it and it works?

@cijothomas
Copy link
Contributor Author

I don't have the data at the TelemetryInitializer stage. My data is calculated and i want to tag the request with this data. I'm interested as to why you think the Activity. Is incorrect. I've tested it and it works?

Because this SDK does't copy Activity Tags into custom properties.
#562
If you see it working, that must be something added by either yourself, or some special integrations. Eg: Azure Functions copy Activity Tags.

TelemetryInitializer is run sync in the same context as the request. So you should have access to all the context when initializers are run.

@sonic1981
Copy link

sonic1981 commented Nov 3, 2020

Well it's working.. I confirmed this, this am.

image

something added by either yourself, or some special integrations. Eg: Azure Functions copy Activity Tags.

100% not done any of this.

@cijothomas
Copy link
Contributor Author

Its done by Functions host.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants