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.

Sign up for GitHub

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

Jump to bottom

Fix AOT warnings in StackExchangeRedis #1415

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Fix AOT warnings in StackExchangeRedis #1415

merged 2 commits into from
Oct 31, 2023

Conversation

eerhardt
Copy link
Contributor

@eerhardt eerhardt commented Oct 31, 2023 •
edited
Loading

  • Add net6.0 target, so we can annotate the code correctly and use new APIs.
  • Copy PropertyFetcher from open-telemetry/opentelemetry-dotnet which was made AOT compatible. Made a new shared version that other libraries can opt into using over time, so this change has no affect on other libraries.
  • Add StackExchangeRedis to the AotCompatibility.TestApp.
  • Address warnings to make the code work under AOT/trimmed.

Fix #1341

cc @Yun-Ting @utpilla @vitek-karas

@eerhardt
Fix AOT warnings in StackExchangeRedis
0bc91ef 
- Add net6.0 target, so we can annotate the code correctly and use new APIs.
- Copy PropertyFetcher from open-telemetry/opentelemetry-dotnet-contrib which was made AOT compatible. Made a new shared version that other libraries can opt into using over time, so this change has no affect on other libraries.
- Add StackExchangeRedis to the AotCompatibility.TestApp.
- Address warnings to make the code work under AOT/trimmed.

Fix open-telemetry#1341
@codecov
Copy link

codecov bot commented Oct 31, 2023 •
edited
Loading

Codecov Report

Merging #1415 (92718cf) into main (71655ce) will decrease coverage by 0.22%.
Report is 40 commits behind head on main.
The diff coverage is 59.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   73.91%   73.69%   -0.22%     
==========================================
  Files         267      261       -6     
  Lines        9615     9631      +16     
==========================================
- Hits         7107     7098       -9     
- Misses       2508     2533      +25
Flag Coverage Δ
unittests-Exporter.Geneva 58.13% <36.84%> (?)
unittests-Exporter.OneCollector 89.71% <ø> (?)
unittests-Extensions 81.73% <ø> (?)
unittests-Instrumentation.AspNet 70.90% <100.00%> (?)
unittests-Instrumentation.EventCounters 75.92% <ø> (?)
unittests-Instrumentation.Owin 85.71% <100.00%> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Runtime 100.00% <ø> (?)
unittests-Instrumentation.StackExchangeRedis 71.37% <6.45%> (?)
unittests-Instrumentation.Wcf 79.01% <82.27%> (?)
unittests-PersistentStorage 58.80% <ø> (?)
unittests-ResourceDetectors.Azure 80.73% <50.00%> (?)
unittests-Solution 80.97% <43.47%> (?)

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

Files Coverage Δ
...orter.Geneva/MsgPackExporter/MsgPackLogExporter.cs 95.10% <100.00%> (+1.76%) ⬆️
...ter.Geneva/MsgPackExporter/MsgPackTraceExporter.cs 94.52% <100.00%> (+2.10%) ⬆️
...ry.Exporter.InfluxDB/InfluxDBExporterExtensions.cs 100.00% <100.00%> (ø)
...ation.AspNet.TelemetryHttpModule/ActivityHelper.cs 85.24% <100.00%> (ø)
...searchClient/ElasticsearchClientInstrumentation.cs 100.00% <100.00%> (ø)
...ityFrameworkCore/EntityFrameworkInstrumentation.cs 100.00% <100.00%> (ø)
...ation/EntityFrameworkInstrumentationEventSource.cs 12.00% <100.00%> (ø)
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 89.42% <100.00%> (-2.44%) ⬇️
....Owin/Implementation/OwinInstrumentationMetrics.cs 100.00% <100.00%> (ø)
...inInstrumentationMeterProviderBuilderExtensions.cs 100.00% <100.00%> (ø)
... and 25 more

... and 33 files with indirect coverage changes

@utpilla utpilla added the comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis label Oct 31, 2023
@eerhardt eerhardt mentioned this pull request Oct 31, 2023
Open
utpilla
utpilla approved these changes Oct 31, 2023
View reviewed changes
Copy link
Contributor

@utpilla utpilla left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG entry.

@eerhardt eerhardt marked this pull request as ready for review October 31, 2023 13:10
@eerhardt eerhardt requested a review from a team October 31, 2023 13:10
@eerhardt
Copy link
Contributor Author

I've addressed all existing feedback. I believe this PR is ready for review.

Yun-Ting
Yun-Ting reviewed Oct 31, 2023
View reviewed changes
src/Shared/PropertyFetcher.AOT.cs

if (declaringType == typeof(object))
{
// TODO: REMOVE this if branch when .NET 7 is out of support.
Copy link
Contributor

Choose a reason for hiding this comment

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

net7.0 is not one of the target for the project currently:
https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/1415/files#:~:text=%3CTargetFrameworks%3Enet6.0%3Bnetstandard2.0%3Bnet462%3C/TargetFrameworks%3E
Do we need this here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like this to be a straight copy from opentelemetry-dotnet. Then we can keep the 2 in sync over time.

Also, the reason this code is here isn't in case one of the libraries targets net7.0, its for when the application targets net7.0, which we can't control here.

@Yun-Ting
Copy link
Contributor

LGTM. Left a nit comment. Thanks!

@utpilla utpilla merged commit 6a3d264 into open-telemetry:main Oct 31, 2023
@eerhardt eerhardt deleted the FixRedisAotWarnings branch October 31, 2023 16:31
@eerhardt eerhardt mentioned this pull request Oct 31, 2023
Merged
eerhardt added a commit to dotnet/aspire that referenced this pull request Oct 31, 2023
@eerhardt
Update Redis OpenTelemetry to latest version (#598)
ac53446 
This brings in AOT support open-telemetry/opentelemetry-dotnet-contrib#1415
eerhardt added a commit to dotnet/aspire that referenced this pull request Oct 31, 2023
@eerhardt
Update Redis OpenTelemetry to latest version (#598)
719b4c3 
This brings in AOT support open-telemetry/opentelemetry-dotnet-contrib#1415
@eerhardt eerhardt mentioned this pull request Oct 31, 2023
Merged
joperezr pushed a commit to dotnet/aspire that referenced this pull request Oct 31, 2023
@eerhardt
Update Redis OpenTelemetry to latest version (#598) (#608)
d6dbfde 
This brings in AOT support open-telemetry/opentelemetry-dotnet-contrib#1415
eerhardt added a commit to eerhardt/opentelemetry-dotnet-contrib that referenced this pull request Oct 31, 2023
@eerhardt
Fix up Redis trimming annotations
dad1cc0 
open-telemetry#1415 had an issue in it due to how the DynamicDependency attribute is written.

1. Referring to a nested type with `+` isn't correct in DynamicDependency attributes. You need to just use a normal `.`. The current code is producing a warning when using the .NET 8.0 SDK to publish for AOT. "warning IL2036: OpenTelemetry.Instrumentation.StackExchangeRedis.Implementation.RedisProfilerEntryToActivityConverter.<.cctor>g__GetCommandAndKey|5_3(PropertyFetcher`1<String>,Object,String&): Unresolved type 'StackExchange.Redis.RedisDatabase+ScriptEvalMessage' in 'DynamicDependencyAttribute'."

This wasn't caught in our CI because we use the .NET 7.0 SDK to publish, which doesn't catch this bug.

2. The "CommandAndKey" property isn't on the nested ScriptEvalMessage type, but instead a virtual on the base Message class.

The fix here is to use the base Message class in the DynamicDependency. I've verified this works with a .NET 8.0 SDK.
@eerhardt eerhardt mentioned this pull request Oct 31, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CodeBlanch CodeBlanch CodeBlanch left review comments

@vitek-karas vitek-karas vitek-karas approved these changes

@Yun-Ting Yun-Ting Yun-Ting approved these changes

@utpilla utpilla utpilla approved these changes

Assignees
No one assigned
Labels
comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make OpenTelemetry.Instrumentation.StackExchangeRedis package AOT safe.
5 participants
@eerhardt @Yun-Ting @vitek-karas @CodeBlanch @utpilla