Skip to content

[Instrumentation.AWS] Support .NET 6 and resolve AoT warning#1547

Merged
Kielek merged 5 commits intoopen-telemetry:mainfrom
martincostello:issue-1543
Jan 26, 2024
Merged

[Instrumentation.AWS] Support .NET 6 and resolve AoT warning#1547
Kielek merged 5 commits intoopen-telemetry:mainfrom
martincostello:issue-1543

Conversation

@martincostello
Copy link
Copy Markdown
Member

Fixes #1543.

Changes

  • Appropriate CHANGELOG.md updated for non-trivial changes

Comment on lines +131 to +158
#if NET6_0_OR_GREATER
[System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage(
"Trimming",
"IL2075",
Justification = "The reflected properties were already used by the AWS SDK's marshallers so the properties could not have been trimmed.")]
#endif
private static void AddRequestSpecificInformation(Activity activity, IRequestContext requestContext, string service)
{
if (AWSServiceHelper.ServiceParameterMap.TryGetValue(service, out string parameter))
if (AWSServiceHelper.ServiceParameterMap.TryGetValue(service, out var parameter))
{
AmazonWebServiceRequest request = requestContext.OriginalRequest;

var property = request.GetType().GetProperty(parameter);
if (property != null)
try
{
if (AWSServiceHelper.ParameterAttributeMap.TryGetValue(parameter, out string attribute))
var property = request.GetType().GetProperty(parameter);
if (property != null)
{
activity.SetTag(attribute, property.GetValue(request));
if (AWSServiceHelper.ParameterAttributeMap.TryGetValue(parameter, out var attribute))
{
activity.SetTag(attribute, property.GetValue(request));
}
}
}
catch (Exception)
{
// Guard against any reflection-related exceptions when running in AoT.
// See https://github.com/open-telemetry/opentelemetry-dotnet-contrib/issues/1543#issuecomment-1907667722.
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codecov
Copy link
Copy Markdown

codecov bot commented Jan 24, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (71655ce) 73.91% compared to head (2e68c2f) 80.62%.
Report is 125 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1547      +/-   ##
==========================================
+ Coverage   73.91%   80.62%   +6.70%     
==========================================
  Files         267      113     -154     
  Lines        9615     3076    -6539     
==========================================
- Hits         7107     2480    -4627     
+ Misses       2508      596    -1912     
Flag Coverage Δ
unittests-Solution 80.62% <88.57%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...Telemetry.Exporter.InfluxDB/InfluxDBEventSource.cs 60.00% <ø> (ø)
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
...metry.Exporter.InfluxDB/InfluxDBMetricsExporter.cs 84.61% <ø> (-3.85%) ⬇️
...Telemetry.Exporter.InfluxDB/PointDataExtensions.cs 87.50% <100.00%> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV1.cs 100.00% <ø> (ø)
...ry.Exporter.InfluxDB/TelegrafPrometheusWriterV2.cs 100.00% <ø> (ø)
...stana/Implementation/InstanaExporterEventSource.cs 0.00% <ø> (ø)
...try.Exporter.Instana/Implementation/InstanaSpan.cs 100.00% <ø> (ø)
...orter.Instana/Implementation/InstanaSpanFactory.cs 100.00% <ø> (ø)
...er.Instana/Implementation/InstanaSpanSerializer.cs 93.79% <ø> (ø)
... and 31 more

... and 225 files with indirect coverage changes

@martincostello martincostello marked this pull request as ready for review January 24, 2024 10:30
@martincostello martincostello requested a review from a team January 24, 2024 10:31
@martincostello
Copy link
Copy Markdown
Member Author

FYI I've deployed a native AoT compiled .NET 8 application using this package to EKS and observed no issues related to this change and the native AoT compilation warning is no longer present.

@Kielek Kielek added the comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS label Jan 24, 2024
Add `net6.0` TFM and fix nullability warnings.
Fix IDE0090 suggestions.
Mark `AWSTracingPipelineHandler` as `sealed` as a hint to the JIT to devirtualize calls.
Update changelog with .NET 6 and AoT changes.
@martincostello
Copy link
Copy Markdown
Member Author

Could I get an ETA on when this will be merged and when there will be a new release of the packages updated by this PR as well as #1545 and #1541?

I've a number of applications where I'd like to consume the changes in as these packages are the blocker for native AoT working in them correctly.

Alternatively, if there's a pre-release feed for NuGet packages from your CI I can consume those in the meantime.

@Kielek
Copy link
Copy Markdown
Member

Kielek commented Jan 26, 2024

Merging as @srprash, agreed that @normj can act as component owner in #1539. There is only some technical requirements to merge mentioned PR.

If we speaking about release - please follow process from https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/CONTRIBUTING.md#how-to-request-for-release-of-package

@Kielek Kielek merged commit a8f7bb8 into open-telemetry:main Jan 26, 2024
@martincostello martincostello deleted the issue-1543 branch January 26, 2024 11:58
@martincostello
Copy link
Copy Markdown
Member Author

@Kielek Thanks - I've raised #1551, though I think the mentioned team is private as it hasn't tagged them due to my permissions.

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

Labels

comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AWSTracingPipelineHandler produces IL2075 warning when compiling for native AoT

4 participants