search

open-telemetry / opentelemetry-dotnet

The OpenTelemetry .NET Client
https://opentelemetry.io
Apache License 2.0
3.23k stars 764 forks source link

Support for .NET 4.5.2 #716

Closed reyang closed 4 years ago

reyang commented 4 years ago

.NET 4.5.2 is still supported by .NET and will be supported until 10/10/2023. I wonder if we need to support it in OpenTelemetry .NET SDK.

The life cycle of .NET 4.5.2 is tight to Windows Server 2012 R2 https://docs.microsoft.com/en-us/lifecycle/faq/dotnet-framework:

Windows Server 2012 R2 extended end of life will happen on 10/10/2023.

Related to #435.

eddynaka commented 4 years ago

I started to add supportability to some of the Exporters. OpenTelemetry.Exporter.Console depends on System.Text.Json, but that doesn't work for .NET 4.5.2. Should I use Newtonsoft instead? If Yes, which version should I add?

cijothomas commented 4 years ago

I started to add supportability to some of the Exporters. OpenTelemetry.Exporter.Console depends on System.Text.Json, but that doesn't work for .NET 4.5.2. Should I use Newtonsoft instead? If Yes, which version should I add? ConsoleExporter is just for local testing, and not official OT exporter. its fine to leave it without 452

eddynaka commented 4 years ago

hi @cijothomas , thanks for the answer.

Next question: .net 452 DateTimeOffset doesn't have ToUnixTimeMilliseconds. What would be a good replace for that?

eddynaka commented 4 years ago

looking at reference source we can see the function logic:

private const int DaysPerYear = 365;
private const int DaysPer4Years = DaysPerYear * 4 + 1;       // 1461
private const int DaysPer100Years = DaysPer4Years * 25 - 1;  // 36524
private const int DaysPer400Years = DaysPer100Years * 4 + 1; // 146097
private const int DaysTo1970 = DaysPer400Years * 4 + DaysPer100Years * 3 + DaysPer4Years * 17 + DaysPerYear; // 719,162
private const long UnixEpochTicks = TimeSpan.TicksPerDay * DaysTo1970; // 621,355,968,000,000,000
private const long UnixEpochMilliseconds = UnixEpochTicks / TimeSpan.TicksPerMillisecond; // 62,135,596,800,000

public static long ToUnixTimeMilliseconds()
{
    // Truncate sub-millisecond precision before offsetting by the Unix Epoch to avoid
    // the last digit being off by one for dates that result in negative Unix times
    long milliseconds = DateTime.UtcNow.Ticks / TimeSpan.TicksPerMillisecond;
    return milliseconds - UnixEpochMilliseconds;
}

Should I add this function somewhere?

reyang commented 4 years ago

One possible way - make an extension method ToUnixTimeMilliseconds and guard it using NET452, scope it so it is only visible to this project.

eddynaka commented 4 years ago

@reyang , where do you suggest to create that?

reyang commented 4 years ago

@reyang , where do you suggest to create that?

https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/extension-methods

eddynaka commented 4 years ago

From what i can see, we still have this projects:

cijothomas commented 4 years ago

For API - i think we can change to net452. There should be no need to support net45 as net45 is no officially supported framework anymore.

@reyang do we have to make Zipkin/Jaeger work for net452 ? Or we can leave them as not supported for net452?

eddynaka commented 4 years ago

@cijothomas , ok, i will open a new pr just updating net452 to api project. for jaeger, the main problem is the task.cancellation. for the other parts, i found a workaround.

reyang commented 4 years ago

@eddynaka @cijothomas regarding Newtonsoft.JSON for 4.5.2, alternative way is JavaScriptSerializer.

reyang commented 4 years ago

For API - i think we can change to net452. There should be no need to support net45 as net45 is no officially supported framework anymore.

@reyang do we have to make Zipkin/Jaeger work for net452 ? Or we can leave them as not supported for net452?

I don't see an immediate need for now. I think net452 should cover API, SDK and integration with .NET (e.g. ASP.NET). Other parts are nice to have but not required.

reyang commented 4 years ago
  • OpenTelemetry.Api: this targets net45, should i change to 452 or add 452?

Given 4.5 and 4.5.1 were deprecated since 2016-01-12, I think it won't make much sense to support them.

eddynaka commented 4 years ago

Hi all,

just to update:

eddynaka commented 4 years ago

@reyang , just to let you know, we ported almost all to 452, except the list below:

Maybe we can close this. What do you think?

reyang commented 4 years ago

Maybe we can close this. What do you think?

Roger that, thanks @eddynaka!

ghost commented 3 years ago

@eddynaka Sorry to trouble you, but is this support complete? It looks like the entire OpenTelemetry.Logs namespace is missing in the 1.1.0 release. If so, can you point to a working example of invoking logging on net452 using open telemetry?

I'm trying to interface my program with an open telemetry exporter we're using, and trying to see what the right way to do that is on net452.

cijothomas commented 3 years ago

There is no logging support in net452. Ms.Extensions.Logging requires minimum net461

ghost commented 3 years ago

@cijothomas thanks for getting back to me quickly.

Microsoft.Extensions.Logging/1.1.2 supports netstandard1.1, including net452. Couldn't you use that? I understand that you wouldn't be able to implement the extension methods, but you should at least be able to implement ILoggerProvider.

Would contributions be welcome here?

cijothomas commented 3 years ago

We cannot take dependency on Ms.Ext.Logging 1.*, as they are no longer supported by Microsoft, so any security issues flagged of them makes OpenTelemetry also vulnerable.

ghost commented 3 years ago

Thanks for clarifying, that seems like a reasonable cause to drop Logging 1.*!

Its sounds like the best course of action for me and other users supporting net452 would be to locally compile our own internal fork of OpenTelemetryLoggerProvider (within the license terms of course). It looks like everything it calls is public already. What do you think?