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

Provide the dependency ID when tracking dependencies #243

Closed
13 tasks done
stijnmoreels opened this issue Oct 12, 2021 · 13 comments
Closed
13 tasks done

Provide the dependency ID when tracking dependencies #243

stijnmoreels opened this issue Oct 12, 2021 · 13 comments
Assignees
Labels
application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies service-to-service-correlation All issues related to service-to-service correlation.
Milestone

Comments

@stijnmoreels
Copy link
Member

stijnmoreels commented Oct 12, 2021

Is your feature request related to a problem? Please describe.
When tracking dependencies, we should include an dependency ID so the later request tracking can use it for its parent operation ID in a service-to-service correlation scenario.

Describe the solution you'd like
We should include an dependencyId to all our available dependencies. This will usually result in method extension overloads.

Additional context
https://github.com/arcus-azure/arcus-service-to-service-correlation-poc

The dependency ID should be added to the following dependency types as extensions overloads:

@stijnmoreels stijnmoreels added application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies service-to-service-correlation All issues related to service-to-service correlation. labels Oct 12, 2021
@stijnmoreels stijnmoreels added this to the v2.4.0 milestone Oct 12, 2021
@stijnmoreels
Copy link
Member Author

As mentioned, we should maybe return the dependency ID when calling .LogDependency so the caller can use the ID somewhere else (like in a HTTP request header).

@tomkerkhove
Copy link
Contributor

The problem is, you will only track the dependency after the request so you have to generate the dependency ID up front and pass it when tracking the dependency so that you can send it to your downstream service.

@stijnmoreels
Copy link
Member Author

The problem is, you will only track the dependency after the request so you have to generate the dependency ID up front and pass it when tracking the dependency so that you can send it to your downstream service.

That's maybe the responsibility of the consumer.

@tomkerkhove
Copy link
Contributor

As mentioned, we should maybe return the dependency ID when calling .LogDependency so the caller can use the ID somewhere else (like in a HTTP request header).

Certainly, but you have to allow them to generate and provide it. If you only return it as suggested here it will not work

@stijnmoreels
Copy link
Member Author

Should we actually add this dependency ID to very dependency call we already have? This means doubling our current set of dependency calls (both with and without a dependency measurement). I'm wondering if it's better to work with options that we can more easily expand.

@tomkerkhove
Copy link
Contributor

Should we actually add this dependency ID to very dependency call we already have? This means doubling our current set of dependency calls (both with and without a dependency measurement).

Yes that is the case, or it could be an optional parameter but then we can have conflicts again so overloads might be best suited

I'm wondering if it's better to work with options that we can more easily expand.

Options are great but add some friction in my opinion as it would make the code more verbose, IMO, but I'm happy to consider it. Would you mind posting a sample?

@stijnmoreels
Copy link
Member Author

logger.LogBlobStorageDependency(
    "<account-name>",
    "<container-name>",
    options =>
    {
        options.IsSuccessful = true;
        options.StartTime = DateTimeOffset.UtcNow;
        options.Duration = TimeSpan.FromSeconds(10);
        options.DependencyId = "<dependency-id>";
    },
    context: new Dictionary<string, object> ());

@tomkerkhove
Copy link
Contributor

This is more verbose imo but I don't have a strong opinion - @fgheysels @gverstraete ?

@stijnmoreels
Copy link
Member Author

Let's keep it as it is, for now. The signature is already complex enough, no need to further complicate it.

@fgheysels
Copy link
Member

I'm not sure if I'm following the use case here. Maybe we can have a short call on this @stijnmoreels ?

@fgheysels
Copy link
Member

fgheysels commented May 17, 2022

@stijnmoreels do we continue with this ?
In my opinion, HttpDependency should have priority here. (It is required to be able to convert the service-to-service correlation poc project to fully rely on Arcus).

@stijnmoreels
Copy link
Member Author

Yes, we continue with this. Was taking this between other work now and then.

@stijnmoreels
Copy link
Member Author

stijnmoreels commented Jun 1, 2022

Great, this monster 👾 of an issue is finally done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-insights All issues related to Azure Application Insights dependencies All issues related to dependencies service-to-service-correlation All issues related to service-to-service correlation.
Projects
None yet
Development

No branches or pull requests

3 participants