[1.3.0 patch] Fix enumerator reflection when using DS7 version #3605
Conversation
|
CI wont run unless we modify the workflows too in this PR.. |
|
The red removed lines from the workflow yaml files in this PR https://github.com/open-telemetry/opentelemetry-dotnet/pull/3531/files might help identify the necessary CI changes |
|
|
||
| static ActivityTagsEnumeratorFactory() | ||
| { | ||
| var activityEventTagsField = typeof(ActivityEvent).GetField("_tags", BindingFlags.Instance | BindingFlags.NonPublic); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here's another area in the OTLP exporter that might risk a similar 💥 in the future
We actually ran into this once before #1854...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Provide a fallback in the event the reflection fails.
- Investigate protobuf-net and consider using it.
- 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
LGTM
Mitigation for the protobuf hack (or not) can be a different PR. See: #3605 (comment)
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| * Added support for the .NET7 version of the | |||
There was a problem hiding this comment.
might be better to call this just 7.0 of DS.
There was a problem hiding this comment.
merging now. Will need to update the changelog to indicate what this hotfix is for.
| - uses: actions/setup-dotnet@v2 | ||
| with: | ||
| dotnet-version: '3.1.x' | ||
|
|
||
| - uses: actions/setup-dotnet@v2 | ||
| with: | ||
| dotnet-version: '6.0.x' |
There was a problem hiding this comment.
nit: can simplify this if desired:
| - 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 |
Fixes #3593
Changes
[Note: This PR is merging into "main-1.3.0" which I created off the 1.3.0 tag. The idea is to release this as a patch 1.3.1(?)]
Patches the 1.3.0 enumerator reflection code to work with .NET7 version of DS.