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

[repo] Centralize TFMs for easier management #2087

Closed
wants to merge 7 commits into from

Conversation

joegoldman2
Copy link
Contributor

Changes

  • Centralize TFMs in Common.props and Common.nonprod.props for easier management (like in the main repo). This change will make it easier to migrate to .NET 9 and future versions.
  • Using common MSBuild properties aligned TFMs between some packages. For example, currently OpenTelemetry.Resources.Azure is targeting netstandard2.0 whereas OpenTelemetry.Resources.AWS don't. I couldn't find any reason why, so I considered it is ok to align.

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

<!-- Tweak style rules for Geneva: Allow underscores in constant names and allow regions inside code blocks -->
<NoWarn>$(NoWarn);SA1123;SA1310</NoWarn>
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
<TargetFrameworks>net8.0;net6.0;netstandard2.0</TargetFrameworks>
<TargetFrameworks Condition="$(OS) == 'Windows_NT'">$(TargetFrameworks);net462</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did the OS check go?

build/Common.props Show resolved Hide resolved
@@ -1,5 +1,4 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<!-- OmniSharp/VS Code requires TargetFrameworks to be in descending order for IntelliSense and analysis. -->
<TargetFrameworks>net8.0;net462</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the net8.0 be replaced with a property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done a6833f7.

@Kielek
Copy link
Contributor

Kielek commented Sep 24, 2024

I didn't make a detailed review, but I have a question why do you want to add netstandard2.0 for all packages?
Part of them is targeting net6.0 and net462. It covers all supported .NET and .NET Framework versions.

@joegoldman2
Copy link
Contributor Author

I didn't make a detailed review, but I have a question why do you want to add netstandard2.0 for all packages? Part of them is targeting net6.0 and net462. It covers all supported .NET and .NET Framework versions.

@CodeBlanch
Copy link
Member

Hey @joegoldman2 we discussed this a bit on our weekly SIG meeting today. Probably more questions came out of the discussion than answers 🤣 Would you be able to join next week or one of the upcoming ones to discuss with us? Calendar is available here: https://calendar.google.com/calendar/u/0/embed?src=c_2bf73e3b6b530da4babd444e72b76a6ad893a5c3f43cf40467abc7a9a897f977@group.calendar.google.com

Look for ".NET SIG" on Tuesdays.

@joegoldman2
Copy link
Contributor Author

Hey! The timezone isn't ideal for me and I'll have to see if I'm available. I'll get back to you.
Otherwise, I had a (very) quick look at the meeting record and I agree with certain things:

  • We can split this PR to make it easier to review it.
  • We can only keep .netstandard2.0 if the project only targets .netstandard2.0 to avoid maintainers to have to add new TFM for each new .NET version.
  • We think it makes sense to mutualize if the project already targets versions like net6.0, as these versions have a relatively short lifespan (1.5 or 3 years) and will have to be update them anyway.

@rgallagher-adc
Copy link

rgallagher-adc commented Sep 25, 2024

Hey! The timezone isn't ideal for me and I'll have to see if I'm available. I'll get back to you. Otherwise, I had a (very) quick look at the meeting record and I agree with certain things:

  • We can split this PR to make it easier to review it.
  • We can only keep .netstandard2.0 if the project only targets .netstandard2.0 to avoid maintainers to have to add new TFM for each new .NET version.
  • We think it makes sense to mutualize if the project already targets versions like net6.0, as these versions have a relatively short lifespan (1.5 or 3 years) and will have to be update them anyway.

Is someone able to explain the downside of targeting .netstandard2.0 ? While comments like @Kielek's "targeting net6.0 and net462... covers all supported .NET and .NET Framework versions." is true if we are thinking about applications, we run into problems when libraries want to incorporate this opentelemetry code. It's a fairly common practice to write libraries that target .NetStandard instead of targeting multiple frameworks to support .NET and .NetFramework apps. However, if my library is targeting .NetStandard, I am unable to include references to packages that do not also target .NetStandard. From my perspective all of the packages that target .NetStandard only are actually significantly more versatile than the packages that target net6.0 and net462 as .NetStandard may be used by all apps and all libraries.

For example, I recently opened up a PR here to add .NetStandard as a TFM to the ConfluentKafka Instrumentation because I am unable to use the instrumentation in my company's Kafka library right now since our kafka library uses .NetStandard, and is nested in call stacks of several libraries all targeting .NetStandard.

@alanwest
Copy link
Member

Here's a summary of my thoughts I shared during our SIG meeting.

  • I do not understand the benefit of aligning packages on a common set of target frameworks. We did this in the opentelemetry-dotnet repo, though I'm not sure there is the same need in the contrib repo. The contrib repo consists of packages with many owners. While the maintainers are here to keep things moving along and make recommendations, I believe the package owners should be responsible for determining what target frameworks make sense for their package.
  • From the PR description: "This change will make it easier to migrate to .NET 9 and future versions." Is this true and how? Supporting .NET 9 and future versions does not necessitate a net9.0 target.
  • Clarifying what we mean by "support": Support means we run the CI against a particular target framework. This does not mean packages must be multi-targeted to that framework.
  • With respect to the questions about .NET Standard. I reminded folks of our statement of support for .NET Standard. I think it would be wise to take this same stance for packages in the contrib repo. As a general rule, we should recommend adding a netstandard2.0 target to any package that does not currently have it. There may be exceptions, but the decision not to add a netstandard2.0 target should be made explicitly and have good reason. I would be supportive of a PR that proposes this change.

@reyang
Copy link
Member

reyang commented Sep 25, 2024

  • I do not understand the benefit of aligning packages on a common set of target frameworks. We did this in the opentelemetry-dotnet repo, though I'm not sure there is the same need in the contrib repo. The contrib repo consists of packages with many owners. While the maintainers are here to keep things moving along and make recommendations, I believe the package owners should be responsible for determining what target frameworks make sense for their package.

+1, I did some work in opentelemetry-dotnet (e.g. open-telemetry/opentelemetry-dotnet#4903, open-telemetry/opentelemetry-dotnet#4911, open-telemetry/opentelemetry-dotnet#4907, open-telemetry/opentelemetry-dotnet#4904) due to a very simple reason: all the projects in that repo are managed by the same set of developers, each time we need to change all of them, so it is better to just consolidate things into a single place and change that single place each time.

For the contrib repo, my understanding is that the components are owned by different folks, if I need to update a package and push a release, I don't want other packages to be updated or released. Similarly, I don't want someone who tries to update their package to push me to release packages that I own.

@joegoldman2
Copy link
Contributor Author

Thank you @alanwest and @reyang for your feedback. It is really interesting. I understand the reasons for not mutualizing targets, as the packages are maintained by different people and you don't want to create strong dependencies between them for all the reasons already mentioned.
I propose few actions for the next steps:

  • I will close this PR.
  • I would like to help adding netstandard2.0 target to all packages that don't target it yet if it is ok for you. This time, I will rather create one PR per project to make them easier to review.
  • Are you ok to mutualize TFM for example apps and test projects?

@reyang
Copy link
Member

reyang commented Sep 26, 2024

  • I would like to help adding netstandard2.0 target to all packages that don't target it yet if it is ok for you. This time, I will rather create one PR per project to make them easier to review.
  • Are you ok to mutualize TFM for example apps and test projects?

Thanks @joegoldman2!

I want us to be very clear on this:

  1. I wear multiple "hats" in the OpenTelemetry project (e.g. I'm a TC member, I'm a maintainer of the security/semconv SIG, I'm listed as an owner of some components in this repository such as OpenTelemetry.Exporter.Geneva).

  2. I am neither a maintainer nor an approver of this particular project/repo.

  3. Whether I am "ok" or "not" can only represent:

    • My personal opinion from technical and logistics perspective.
    • My ownership position as the owner of specific components (e.g. OpenTelemetry.Exporter.Geneva), and the corresponding tests, docs and examples.
    • My answer to your questions would be 1) I'm okay - actually I don't care, I'm just not against it at all 2) I'm okay - I think that's a good thing that will bring some benefits so there seems to be net gain IMHO

    What I cannot represent:

    • The owners of other components in this repository.
    • The maintainers who are in charge of the general direction, such as whether to unify test infrastructure, or require all examples to support certain TFM.

    I think the maintainers also have the power to enforce certain policy across all component owners, I feel they probably don't want to enforce TFM or lock step all the releases though.

@Kielek
Copy link
Contributor

Kielek commented Sep 26, 2024

  • I would like to help adding netstandard2.0 target to all packages that don't target it yet if it is ok for you. This time, I will rather create one PR per project to make them easier to review.

I am not sure if it is good for all cases. Keep in mind that some features/improvements are available only for .NET6/8+ targets. You need to be very careful when you consider adding legacy .NET Standard 2.0.
Handling it one, be one is better option than putting all of them in one batch.

There is one more important thing. How to test such target? Most of the libraries already targeting .net6.0 and .net462. In such cases these versions will be taken to unit tests projects.

  • Are you ok to mutualize TFM for example apps and test projects?

Examples - I would say yes. Most of them should be pinned to net8.0. You need to only verify Asp.net, owin and wcf.

Tests, probably we need only targets executed during the pipeline (.net8, .net6 (will be dropped in November), .net462, shortly .net9). There is a open question what to do with additional targets (like for Geneva).
It will be good to do it also one by one, together with modifying standard target frameworks. If done in this way, it can be reviewed by the component owners.

@joegoldman2
Copy link
Contributor Author

I am not sure if it is good for all cases. Keep in mind that some features/improvements are available only for .NET6/8+ targets. You need to be very careful when you consider adding legacy .NET Standard 2.0.

I think we can add netstandard2.0 without removing .NET8+ targets, and use preprocessor directives to take advantage of new features that are not available through .NET Standard, right?

@joegoldman2
Copy link
Contributor Author

joegoldman2 commented Sep 27, 2024

One additional question: do we want to start replacing .NET 6 target with .NET 8 (it can be done at the time as adding .NET Standard 2.0 target)?

@reyang
Copy link
Member

reyang commented Sep 27, 2024

One additional question: do we want to start replacing .NET 6 target with .NET 8 (it can be done at the time as adding .NET Standard 2.0 target)?

👍💯

@joegoldman2 joegoldman2 deleted the feat/tfm branch September 28, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.geneva Things related to OpenTelemetry.Exporter.Geneva comp:exporter.influxdb Things related to OpenTelemetry.Exporter.InfluxDB comp:exporter.instana Things related to OpenTelemetry.Instrumentation.Instana comp:exporter.onecollector Things related to OpenTelemetry.Exporter.OneCollector comp:exporter.stackdriver Things related to OpenTelemetry.Exporter.Stackdriver comp:extensions.aws Things related to OpenTelemetry.Extensions.AWS comp:extensions.enrichment Things related to OpenTelemetry.Extensions.Enrichment comp:extensions Things related to OpenTelemetry.Extensions comp:instrumentation.aspnet.telemetryhttpmodule Things related to OpenTelemetry.Instrumentation.AspNet.TelemetryHttpModule comp:instrumentation.aspnet Things related to OpenTelemetry.Instrumentation.AspNet comp:instrumentation.aspnetcore Things related to OpenTelemetry.Instrumentation.AspNetCore comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda comp:instrumentation.cassandra Things related to OpenTelemetry.Instrumentation.Cassandra comp:instrumentation.confluentkafka Things related to OpenTelemetry.Instrumentation.ConfluentKafka comp:instrumentation.elasticsearchclient Things related to OpenTelemetry.Instrumentation.ElasticsearchClient comp:instrumentation.entityframeworkcore Things related to OpenTelemetry.Instrumentation.EntityFrameworkCore comp:instrumentation.eventcounters Things related to OpenTelemetry.Instrumentation.EventCounters comp:instrumentation.grpccore Things related to OpenTelemetry.Instrumentation.GrpcCore comp:instrumentation.grpcnetclient Things related to OpenTelemetry.Instrumentation.GrpcNetClient comp:instrumentation.hangfire Things related to OpenTelemetry.Instrumentation.Hangfire comp:instrumentation.http Things related to OpenTelemetry.Instrumentation.Http comp:instrumentation.owin Things related to OpenTelemetry.Instrumentation.Owin comp:instrumentation.process Things related to OpenTelemetry.Instrumentation.Process comp:instrumentation.quartz Things related to OpenTelemetry.Instrumentation.Quartz comp:instrumentation.runtime Things related to OpenTelemetry.Instrumentation.Runtime comp:instrumentation.sqlclient Things related to OpenTelemetry.Instrumentation.SqlClient comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf comp:persistentstorage.abstractions Things related to OpenTelemetry.PersistentStorage.Abstractions comp:persistentstorage.filesystem Things related to OpenTelemetry.PersistentStorage.FileSystem comp:resources.aws Things related to OpenTelemetry.Resources.AWS comp:resources.azure Things related to OpenTelemetry.Resources.Azure comp:resources.container Things related to OpenTelemetry.Resources.Container comp:resources.gcp Things related to OpenTelemetry.Resources.Gcp comp:resources.host Things related to OpenTelemetry.Resources.Host comp:resources.operatingsystem Things related to OpenTelemetry.Resources.OperatingSystem comp:resources.process Things related to OpenTelemetry.Resources.Process comp:resources.processruntime Things related to OpenTelemetry.Resources.ProcessRuntime comp:sampler.aws Things related to OpenTelemetry.Samplers.AWS comp:semanticconventions Things related to OpenTelemetry.SemanticConventions documentation Improvements or additions to documentation infra Infra work - CI/CD, code coverage, linters perf Performance related
Projects
None yet
Development

Successfully merging this pull request may close these issues.