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

Support Activity.Tags #562

Open
lmolkova opened this issue May 22, 2017 · 14 comments
Open

Support Activity.Tags #562

lmolkova opened this issue May 22, 2017 · 14 comments

Comments

@lmolkova
Copy link
Member

@lmolkova lmolkova commented May 22, 2017

Activity.Tags is a property bag with string key value pairs. Tags only belong to current activity and does not flow to the child activities (internal or external).

ApplicationInsights should copy them to telemetry properties

Note that Tags should be copied only to the operation that is represented by current Activity and not to the nested telemetries tracked in scope of this operation.

@lmolkova
Copy link
Member Author

@lmolkova lmolkova commented Mar 29, 2018

If it will be decided to tag telemetry with all parents Activities tags, similar to

var activity = Activity.Current;
while (activity != null) {
   // set all tags on telemetry
  activity = activity.Parent;
}

such code may introduce endless loop if the stack of activities is broken (contains a cycle).
Here aspnet/Microsoft.AspNet.TelemetryCorrelation#23 we have introduced a hard limit on nested Activities (128) to protect from this hypothetical issue.

While it does not seem to be possible now, it's likely that Activity.Current setter will become public and we should expect any kinds of weird stacks in future.

@TimothyMothra TimothyMothra modified the milestones: 2.6-Beta3, 2.7 Apr 16, 2018
@lmolkova lmolkova self-assigned this Apr 17, 2018
@TimothyMothra
Copy link
Member

@TimothyMothra TimothyMothra commented Apr 19, 2018

@lmolkova Do you want to address this within 2.7 milestone or assign to "Future" ?

@lmolkova lmolkova modified the milestones: 2.7, Future Apr 19, 2018
@Expecho
Copy link
Contributor

@Expecho Expecho commented Sep 16, 2020

@lmolkova , @TimothyMothra I just noticed that inside a .Net Core 3.1 based Azure Function this is already happening. Given the code below a custom property is added to the request telemetry.

        [FunctionName("HttpTriggered")]
        public IActionResult Run(
            [HttpTrigger(AuthorizationLevel.Function, "get", "post", Route = null)] HttpRequest req)
        {
            Activity.Current.AddTag("aTag", "aValue");
            return new OkResult();
        }

when I tried the same inside a web api controller hosted in an azure web app it does not work.
What is the status of this issue?

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Sep 16, 2020

This is supported in Azure Functions, but not in the main ApplicationInsights SDK shipped from this repo.
At this stage the recommendation is to write own TelemetryInitializer to populate Activity.Tags into ITelemetry.Properties.

There will be tighter integration between Activity and ApplicationInsights (via OpenTelemetry), I believe this would be addressed in the future, but not planned for 2020.

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Sep 16, 2020

@Expecho Did you use Activity.Current.AddTag("aTag", "aValue"); as a way to add custom properties to an auto-collected telemetry in codeless attach? (There might be support for this coming in a future version of codeless attach, though there is no timeline to this yet)

@Expecho
Copy link
Contributor

@Expecho Expecho commented Sep 16, 2020

@Expecho Did you use Activity.Current.AddTag("aTag", "aValue"); as a way to add custom properties to an auto-collected telemetry in codeless attach? (There might be support for this coming in a future version of codeless attach, though there is no timeline to this yet)

I use it to add custom properties with values that are not accessible inside a TelemetryInitializer.

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Sep 16, 2020

Got it... Can you give example of such values.. Where are you retrieving it?

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Sep 16, 2020

Since you are using SDK, the following can also work.

var telemetry = httpContext?.Features.Get<RequestTelemetry>();
if (telemetry != null)
{
   telemetry.Properties.Add("key", "value");
}
                
@Expecho
Copy link
Contributor

@Expecho Expecho commented Sep 21, 2020

Since you are using SDK, the following can also work.

var telemetry = httpContext?.Features.Get<RequestTelemetry>();
if (telemetry != null)
{
   telemetry.Properties.Add("key", "value");
}
                

Yes, I know. But this does not work in an EventGrid event triggered Azure Function for example, because there is no http context/request we can access.

I am about to give a demo to some collegues about how to enrich telemetry and copied some code from an azure function and realized it did not work based on Activity.Current. Then I tried to find out where this difference of behavior comes from. Now I know :-)

For azure functions we will stick to Activity when we have no request/context available and for all other cases we use ttpContext?.Features.

Sometimes we want to have those properties attached to all telemetry so it easier to query. For example, say we have an exception in some eventgrid triggered azure function. It is easies to know which event caused it by looking at the eventid custom property than to first have to find the request that triggered the processing.

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Sep 21, 2020

Just listing down current state:

  1. For SDK usage in non-Functions - have to write own initializer, and copy things from Activity.Tags manually to Property.
  2. Azure Functions does Tags -> Properties automatically.
  3. Codeless - no option to add additional property exists today.

I totally understand the use case :) Not planning to make any changes to SDK now to handle this, but this would be covered when we do OpenTelemetry integration, when Activity will be used for representing every operation.

@SeanFeldman
Copy link

@SeanFeldman SeanFeldman commented Jan 1, 2021

Just listing down current state:

  1. For SDK usage in non-Functions - have to write own initializer, and copy things from Activity.Tags manually to Property.
  2. Azure Functions does Tags -> Properties automatically.
  3. Codeless - no option to add additional property exists today.

I'm using Azure Functions (v3, netcoreapp3.1, Microsoft.Azure.WebJobs.Logging.ApplicationInsights 3.0.18), adding Tag and Baggage and it doesn't work, @cijothomas. Despite the second item in the list, saying it should work. Do I need to follow the suggestion of writing my own TelemetryInitializer?

@cijothomas
Copy link
Contributor

@cijothomas cijothomas commented Jan 4, 2021

@SeanFeldman If you are facing issues with Functions integration with ApplicationInsihgts, please open an issue in the Functions repo. https://github.com/Azure/azure-webjobs-sdk

@Expecho
Copy link
Contributor

@Expecho Expecho commented Jan 4, 2021

@SeanFeldman are you sure it does not work? Because I confirmed this to be working some time ago, see https://github.com/Ibis-Software/AppInsightsDemo/blob/master/src/FunctionApp/HttpTriggered.cs

@SeanFeldman
Copy link

@SeanFeldman SeanFeldman commented Jan 5, 2021

@Expecho I'm not tracking custom events. In my Service Bus triggered Function, I'm logging and expect to see those tags to be added to the custom dictionary entries on the log statement captured with traces. At some point in time, it was working just fine (Tags and Baggage) but then it stopped.

I'll create a stand-alone repro to see if I can repeat it in a fresh project and raise an issue in https://github.com/Azure/azure-webjobs-sdk, just like @cijothomas has recommended.

@reyang reyang removed this from the Future milestone Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants