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

[1.3.0 patch] Fix enumerator reflection when using DS7 version #3605

Merged
merged 5 commits into from
Aug 31, 2022
Merged

[1.3.0 patch] Fix enumerator reflection when using DS7 version #3605

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
632f202
Patch to fix DS7 with 1.3.0 style enumerator reflection.
CodeBlanch Aug 25, 2022
61af155
Attempt to fix whitespace on diff.
CodeBlanch Aug 25, 2022
9a48648
Attempting to get some of the CI running for branch.
CodeBlanch Aug 25, 2022
7278031
More CI tweaks.
CodeBlanch Aug 25, 2022
f04e43d
CHANGELOG update.
CodeBlanch Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions .github/workflows/linux-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ name: Linux

on:
push:
branches: [ main ]
branches: [ 'main*' ]
paths-ignore:
- '**.md'
pull_request:
branches: [ main ]
branches: [ 'main*' ]
paths-ignore:
- '**.md'

Expand All @@ -16,11 +16,24 @@ jobs:

strategy:
matrix:
version: [netcoreapp3.1,net6.0]
version: [netcoreapp3.1,net6.0,net7.0]

steps:
- uses: actions/checkout@v3

- uses: actions/setup-dotnet@v2
with:
dotnet-version: '3.1.x'

- uses: actions/setup-dotnet@v2
with:
dotnet-version: '6.0.x'

- uses: actions/setup-dotnet@v2
with:
dotnet-version: '7.0.x'
include-prerelease: true

- name: Install dependencies
run: dotnet restore

Expand Down
19 changes: 16 additions & 3 deletions .github/workflows/windows-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ name: Windows

on:
push:
branches: [ main ]
branches: [ 'main*' ]
paths-ignore:
- '**.md'
pull_request:
branches: [ main ]
branches: [ 'main*' ]
paths-ignore:
- '**.md'

Expand All @@ -16,11 +16,24 @@ jobs:

alanwest marked this conversation as resolved.
Show resolved Hide resolved
strategy:
matrix:
version: [net462,netcoreapp3.1,net6.0]
version: [net462,netcoreapp3.1,net6.0,net7.0]

steps:
- uses: actions/checkout@v3

- uses: actions/setup-dotnet@v2
with:
dotnet-version: '3.1.x'

- uses: actions/setup-dotnet@v2
with:
dotnet-version: '6.0.x'
Comment on lines +24 to +30
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can simplify this if desired:

Suggested change
- uses: actions/setup-dotnet@v2
with:
dotnet-version: '3.1.x'
- uses: actions/setup-dotnet@v2
with:
dotnet-version: '6.0.x'
- uses: actions/setup-dotnet@v2
with:
dotnet-version: |
3.1.x
6.0.x


- uses: actions/setup-dotnet@v2
with:
dotnet-version: '7.0.x'
include-prerelease: true

- name: Install dependencies
run: dotnet restore

Expand Down
3 changes: 2 additions & 1 deletion OpenTelemetry.sln
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Solution Items", "Solution
.dockerignore = .dockerignore
.editorconfig = .editorconfig
CONTRIBUTING.md = CONTRIBUTING.md
global.json = global.json
LICENSE = LICENSE
NuGet.config = NuGet.config
OpenTelemetry.proj = OpenTelemetry.proj
Expand Down Expand Up @@ -233,7 +234,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Pr
EndProject
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Propagators.Tests", "test\OpenTelemetry.Extensions.Propagators.Tests\OpenTelemetry.Extensions.Propagators.Tests.csproj", "{476D804B-BFEC-4D34-814C-DFFD97109989}"
EndProject
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "correlation", "docs\logs\correlation\correlation.csproj", "{9A07D215-90AC-4BAF-BCDB-73D74FD3A5C5}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "correlation", "docs\logs\correlation\correlation.csproj", "{9A07D215-90AC-4BAF-BCDB-73D74FD3A5C5}"
EndProject
Global
GlobalSection(SolutionConfigurationPlatforms) = preSolution
Expand Down
2 changes: 1 addition & 1 deletion build/Common.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
<StackExchangeRedisPkgVer>[2.1.58,3.0)</StackExchangeRedisPkgVer>
<StyleCopAnalyzersPkgVer>[1.2.0-beta.354,2.0)</StyleCopAnalyzersPkgVer>
<SystemCollectionsImmutablePkgVer>1.4.0</SystemCollectionsImmutablePkgVer>
<SystemDiagnosticSourcePkgVer>6.0.0</SystemDiagnosticSourcePkgVer>
<SystemDiagnosticSourcePkgVer>[6.0.0,8.0)</SystemDiagnosticSourcePkgVer>
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
<SystemReflectionEmitLightweightPkgVer>4.7.0</SystemReflectionEmitLightweightPkgVer>
<SystemTextJsonPkgVer>4.7.0</SystemTextJsonPkgVer>
<SystemThreadingTasksExtensionsPkgVer>4.5.3</SystemThreadingTasksExtensionsPkgVer>
Expand Down
5 changes: 3 additions & 2 deletions global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
{
"sdk": {
"rollForward": "latestFeature",
"version": "6.0.100"
"rollForward": "latestMajor",
"version": "6.0.100",
"allowPrerelease": true
}
}
36 changes: 31 additions & 5 deletions src/OpenTelemetry.Api/Internal/ActivityHelperExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,11 +259,37 @@ private static readonly DictionaryEnumerator<string, object, TState>.AllocationF
ActivityTagObjectsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(
typeof(Activity).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic).FieldType);

private static readonly DictionaryEnumerator<string, object, TState>.AllocationFreeForEachDelegate
ActivityTagsCollectionEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(typeof(ActivityTagsCollection));

private static readonly DictionaryEnumerator<string, object, TState>.ForEachDelegate ForEachTagValueCallbackRef = ForEachTagValueCallback;

private static readonly DictionaryEnumerator<string, object, TState>.AllocationFreeForEachDelegate ActivityEventTagsEnumerator;

private static readonly DictionaryEnumerator<string, object, TState>.AllocationFreeForEachDelegate ActivityLinkTagsEnumerator;

static ActivityTagsEnumeratorFactory()
{
var activityEventTagsField = typeof(ActivityEvent).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic);
Copy link
Member

Choose a reason for hiding this comment

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

Since we're going through with a patch release, I think it makes sense to implement the ultimate fallback in the event things change beyond DS 7. Also could put the defense in for Activity tags as well if ever that were to change.

Copy link
Member

Choose a reason for hiding this comment

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

Here's another area in the OTLP exporter that might risk a similar 💥 in the future

opentelemetry-dotnet/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/Implementation/ActivityExtensions.cs

Line 300 in 28c0bcc

private static Action<RepeatedField<Span>, int> CreateRepeatedFieldOfSpanSetCountAction()

We actually ran into this once before #1854...

Copy link
Member

Choose a reason for hiding this comment

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

Since we're going through with a patch release, I think it makes sense to implement the ultimate fallback in the event things change beyond DS 7. Also could put the defense in for Activity tags as well if ever that were to change.

+1

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's another area in the OTLP exporter that might risk a similar 💥 in the future

I actually pinged the protobuf team the other day about if they would accept a contribution to make that OTLP hack/feature part of the public API. I don't want to link it here because it is kind of off topic but the response seemed like a maybe? I was going to throw a PR up but then @reyang pointed out to me there is an alternative thing protobuf-net which seems to use built-in List<T> for its codegen. Might make more sense to switch libs completely. Needs a bit of investigation so I tabled it for now.

Copy link
Member

@alanwest alanwest Aug 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Needs a bit of investigation so I tabled it for now.

I think this is fine, but before pushing out the patch release I think we should decide what our strategy is here. The protobuf hack is not a problem today, but could be tomorrow.

So question is: Do we want to mitigate for this in the 1.3.1 patch release?

I think the answer is yes.

If other agree, then sounds like options include:

  1. Provide a fallback in the event the reflection fails.
  2. Investigate protobuf-net and consider using it.
  3. Wait for protobuf team to release real support for OTLP hack.

I'd be content with option 1. It requires no further investigation and sufficiently guards users of 1.3.1 from any surprise application crashes when upgrading their protobuf/gRPC dependencies in the future.

Copy link
Member

Choose a reason for hiding this comment

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

switching to protobuf-net, if we do it, should be part of regular minor version bump, not ".1" patch for addressing bug fix.

A fallback would be the best option here.

if (activityEventTagsField != null)
{
// .NET 7 API
ActivityEventTagsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(activityEventTagsField.FieldType);
}
else
{
ActivityEventTagsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(typeof(ActivityTagsCollection));
}

var activityLinkTagsField = typeof(ActivityLink).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic);
if (activityLinkTagsField != null)
{
// .NET 7 API
ActivityLinkTagsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(activityLinkTagsField.FieldType);
}
else
{
ActivityLinkTagsEnumerator = DictionaryEnumerator<string, object, TState>.BuildAllocationFreeForEachDelegate(typeof(ActivityTagsCollection));
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Enumerate(Activity activity, ref TState state)
{
Expand All @@ -290,7 +316,7 @@ public static void Enumerate(ActivityLink activityLink, ref TState state)
return;
}

ActivityTagsCollectionEnumerator(
ActivityLinkTagsEnumerator(
tags,
ref state,
ForEachTagValueCallbackRef);
Expand All @@ -306,7 +332,7 @@ public static void Enumerate(ActivityEvent activityEvent, ref TState state)
return;
}

ActivityTagsCollectionEnumerator(
ActivityEventTagsEnumerator(
tags,
ref state,
ForEachTagValueCallbackRef);
Expand Down
5 changes: 3 additions & 2 deletions test/OpenTelemetry.Tests/OpenTelemetry.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
<PropertyGroup>
<Description>Unit test project for OpenTelemetry</Description>
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
<TargetFrameworks>net6.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks>net7.0;net6.0;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net462</TargetFrameworks>
<NoWarn>$(NoWarn),CS0618</NoWarn>
</PropertyGroup>
Expand All @@ -13,7 +13,8 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" />
<PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" />
<PackageReference Include="System.Diagnostics.DiagnosticSource" Version="7.0.0-preview.7.22375.6" Condition="$(TargetFramework) == 'net7.0'" />
</ItemGroup>

<ItemGroup>
Expand Down
2 changes: 1 addition & 1 deletion test/OpenTelemetry.Tests/Trace/LinkTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ public void Equality()
Assert.True(link1.Equals(link3));
}

[Fact]
[Fact(Skip = "NET7 doesn't support ActivityLink equality")]
public void Equality_WithAttributes()
{
var link1 = new Link(this.spanContext, this.tags);
Expand Down