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

[sdk] Remove System.Reflection.Emit.Lightweight dependency #4140

Merged
merged 4 commits into from
Feb 2, 2023

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Feb 2, 2023

Changes

  • Removes System.Reflection.Emit.Lightweight dependency from SDK (it is no longer needed).
  • Adds System.Reflection.Emit.Lightweight dependency to OpenTelemetry.Exporter.OpenTelemetryProtocol where it is currently needed.

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes

@CodeBlanch CodeBlanch requested a review from a team February 2, 2023 17:54
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #4140 (1da1961) into main (c0f927f) will decrease coverage by 0.06%.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4140      +/-   ##
==========================================
- Coverage   85.54%   85.48%   -0.06%     
==========================================
  Files         293      293              
  Lines       11371    11371              
==========================================
- Hits         9727     9721       -6     
- Misses       1644     1650       +6     
Impacted Files Coverage Δ
...tation/OpenTelemetryProtocolExporterEventSource.cs 85.00% <0.00%> (-15.00%) ⬇️
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs 72.72% <0.00%> (-13.64%) ⬇️
...lementation/SqlClientInstrumentationEventSource.cs 70.83% <0.00%> (-4.17%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...tpListener/Internal/PrometheusCollectionManager.cs 73.62% <0.00%> (-2.20%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+8.82%) ⬆️

Copy link
Contributor

@Kielek Kielek left a comment

Choose a reason for hiding this comment

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

Consider to put information about direct dependency i OTLP CHANGELOG (I know that it was transitively available).

<PackageReference Include="Google.Protobuf" Version="$(GoogleProtobufPkgVer)" />
<PackageReference Include="Grpc.Tools" Version="$(GrpcToolsPkgVer)" PrivateAssets="all" />
<PackageReference Include="System.Reflection.Emit.Lightweight" Version="$(SystemReflectionEmitLightweightPkgVer)" Condition="'$(TargetFramework)' != 'net6.0'" />
Copy link
Member Author

Choose a reason for hiding this comment

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

For those curious, OTLP Exporter uses a couple dynamic methods to reset the count of RepeatedFields so they may be reused to avoid some allocations. Example here. A while back we were able to get a change put into Protobuf to make the default clear behavior capacity-preserving. I just checked, there is 3.22.0-rc1 which includes the change. So it seems we will be able to clean this up soon!

@CodeBlanch CodeBlanch merged commit da938d7 into open-telemetry:main Feb 2, 2023
@CodeBlanch CodeBlanch deleted the sdk-reflectionemit branch February 2, 2023 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants