Aws sdk api phase1 #3401
Aws sdk api phase1 #3401
Conversation
| private readonly PluginManager _pluginManager; | ||
|
|
||
| public AWSInitializer(PluginManager pluginManager) | ||
| : base("OpenTelemetry.Instrumentation.AWS") |
There was a problem hiding this comment.
@birojnayak, I think that this line is wrong.
You are listening on "OpenTelemetry.Instrumentation.AWS" be loaded by application. It should never happen.
I suppose that you need put here AWSSDK.DynamoDBv2 or any other library you want to instrument (instead of instrumentation library).
There was a problem hiding this comment.
I added the same but still don't see any aws span.. here is the gist
There was a problem hiding this comment.
@Kielek made it work.. the culprit was the initialization.
| @@ -60,6 +61,9 @@ | |||
|
|
|||
| <ItemGroup Label="Transient dependencies auto-generated by GenerateNetFxTransientDependencies" Condition=" '$(TargetFramework)' == 'net462' AND $(_IsPacking) != true "> | |||
| <!-- These dependencies are also not included in the NuGet package since the conflicts can be left to NuGet version resolution. --> | |||
| <PackageReference Include="AWSSDK.Core" /> | |||
There was a problem hiding this comment.
Are these OpenTelemetry Instrumentation libraries for AWS?
There was a problem hiding this comment.
@rajkumar-rangaraj I believe these are dependent packages as here
There was a problem hiding this comment.
Vendor library references should not be a part of this project, as this is against the project's principles. These references could be included in the tests, but not in the main project.
There was a problem hiding this comment.
@rajkumar-rangaraj TBH, I didn't understand fully what's vendor library, I assumed for each service you can add a package to get trace. But, I fully understand there are 4 libraries we are adding and bringing those dependencies, so I was asking in the SIG (last 2 meetings) and checking with everyone if it's ok.
The other option is to enable via Plugin(I have to validate that too), but in that case it would be in it's own repo with many other attach(exporter, agent etc), which won't benefit broader community (AFAIK) . Let's discuss what would be ideal in SIG.
There was a problem hiding this comment.
So the main issue here is that the OpenTelemetry.Instrumentation.AWS NuGet package references other AWS libraries. For our purposes, this is not acceptable because we should not be bringing in other libraries. Compare this to the OpenTelemetry.Instrumentation.Quartz package which only references the OpenTelemetry.Api
I think this prevents us from taking the change, until the AWS instrumentation package can be rewritten in a way that doesn't require referencing the AWS packages.
There was a problem hiding this comment.
I understand guys and in line with this.... at the same time let's discuss about any alternative...
birojnayak
force-pushed
the
aws-sdk-api-phase1
branch
from
9b1069e
to
9f2ad81
Compare
birojnayak
force-pushed
the
aws-sdk-api-phase1
branch
from
9f2ad81
to
5bc7b29
Compare
birojnayak
force-pushed
the
aws-sdk-api-phase1
branch
from
5bc7b29
to
8fa44db
Compare
|
@Kielek , @rajkumar-rangaraj tests those are failing here doesn't look related to my change, let me know if I am missing something. 17:47:41 [DBG] [xUnit.net 00:04:20.05] IntegrationTests: === RUN IntegrationTests.BuildTests.DistributionStructure()
Error: [xUnit.net 00:04:20.07] IntegrationTests.BuildTests.DistributionStructure [FAIL]
17:47:41 [ERR] [xUnit.net 00:04:20.07] IntegrationTests.BuildTests.DistributionStructure [FAIL]
17:47:41 [DBG] [xUnit.net 00:04:20.07] IntegrationTests: --- FAIL: IntegrationTests.BuildTests.DistributionStructure() (0.01s) |
It is fully related. These changes brings at least 5 dependencies to the project not included into
This set of tests protect us from including unwanted dependencies (things mentioned by Raj here. |
| { | ||
| var instrumentationType = Type.GetType("OpenTelemetry.Instrumentation.AWS.Implementation.AWSClientsInstrumentation, OpenTelemetry.Instrumentation.AWS")!; | ||
|
|
||
| var awsClientOptions = new AWSClientInstrumentationOptions(); |
There was a problem hiding this comment.
@rajkumar-rangaraj @birojnayak I think we could try experimenting with the following:
- Creating this instrumentation options type dynamically
Remove the(see my below edit)OpenTelemetry.Instrumentation.AWSnuget package reference
What do you think?
There was a problem hiding this comment.
Actually, I do believe we'll need the OpenTelemetry.Instrumentation.AWS package as we should not be expecting the user to provide this. As a result, we would need to do the same dependency trimming as MongoDb so we only bring in the OpenTelemetry.Instrumentation.AWS DLL and not its dependencies. However if we create the instrumentation options dynamically then we shouldn't get the other TypeReferences and AssemblyReferences.
Does this approach sound reasonable to you @Kielek?
There was a problem hiding this comment.
It is ugly (exactly as for MongoDB) and put additional cost for militance, but I agree that it is the best solution we can find.
While implementing/updating package we need to be extremely carefully about exact version to remove from our project.
There was a problem hiding this comment.
so what's the verdict guys ? I am open to what team decides.. (sorry couldn't attend the SIG Today).
|
We are implementing internally in the plugins, so closing this. We do have plan to have 1 package for AWS SDK related changes, once that is out, I will re-submit another PR. |
Why
Enabling AWS SDK Calls
What
Re-using OpenTelemetry.Instrumentation.AWS and bringing SDK api calls to auto instrumentation
Tests
Added integration test
Checklist
CHANGELOG.mdis updated.