Copilot commented on code in PR #4243:
URL: https://github.com/apache/arrow-adbc/pull/4243#discussion_r3106233987
##########
csharp/src/Apache.Arrow.Adbc/Tracing/ActivityTrace.cs:
##########
@@ -263,6 +264,28 @@ public void Dispose()
ActivitySource.Dispose();
}
+ /// <summary>
+ /// If possible, gets the file version for the assembly associated
with the given Type.
+ /// </summary>
+ [SuppressMessage("SingleFile", "IL3000", Justification="Using guard")]
+ public static string GetAssemblyVersion(Type type)
+ {
+ var versionAttr =
type.Assembly.GetCustomAttribute<AssemblyInformationalVersionAttribute>();
+ if (versionAttr?.InformationalVersion != null) return
versionAttr.InformationalVersion;
+
+#if NET8_0_OR_GREATER
+ if (RuntimeFeature.IsDynamicCodeSupported &&
type.Assembly.Location != null)
+ {
+ var fileVersion =
FileVersionInfo.GetVersionInfo(type.Assembly.Location).ProductVersion;
+ if (fileVersion != null) return fileVersion;
+ }
Review Comment:
`Assembly.Location` is an empty string (not null) under single-file publish,
so the guard `type.Assembly.Location != null` won't prevent
`FileVersionInfo.GetVersionInfo(...)` from being called and can still throw.
Consider guarding with `!string.IsNullOrEmpty(type.Assembly.Location)` (or
`!string.IsNullOrWhiteSpace`) before calling into `FileVersionInfo`.
##########
csharp/src/Telemetry/Traces/Listeners/FileListener/OtelAttributesConverter.cs:
##########
@@ -0,0 +1,204 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+using System;
+using System.Collections;
+using System.Collections.Generic;
+using System.Globalization;
+using System.Text.Json;
+using System.Text.Json.Serialization;
+
+namespace Apache.Arrow.Adbc.Telemetry.Traces.Listeners.FileListener
+{
+ /// <summary>
+ /// Writes OTel-compatible attribute values as native JSON. The
OpenTelemetry spec allows
+ /// attribute values to be <c>string</c>, <c>bool</c>, <c>long</c>, or
<c>double</c>, or
+ /// homogeneous arrays of those types. Values of those types are emitted
as native JSON
+ /// scalars/arrays. Anything else (DateTime, custom types, etc.) falls
through to an
+ /// invariant-culture string representation so the serialized form doesn't
vary by locale.
+ /// </summary>
+ internal static class OtelAttributeWriter
+ {
+ public static void WriteValue(Utf8JsonWriter writer, object? value)
+ {
+ switch (value)
+ {
+ case null:
+ writer.WriteNullValue();
+ return;
+ case string s:
+ writer.WriteStringValue(s);
+ return;
+ case bool b:
+ writer.WriteBooleanValue(b);
+ return;
+ // OTel's integer attributes are int64. Any integral type up
to long fits.
+ case long l:
+ writer.WriteNumberValue(l);
+ return;
+ case int i:
+ writer.WriteNumberValue(i);
+ return;
+ case short sh:
+ writer.WriteNumberValue(sh);
+ return;
+ case byte by:
+ writer.WriteNumberValue(by);
+ return;
+ case sbyte sb:
+ writer.WriteNumberValue(sb);
+ return;
+ case uint ui:
+ writer.WriteNumberValue(ui);
+ return;
+ case ulong ul:
+ writer.WriteNumberValue(ul);
+ return;
+ case ushort us:
+ writer.WriteNumberValue(us);
+ return;
+ // OTel floating-point is double; float widens without loss.
+ case double d:
+ writer.WriteNumberValue(d);
+ return;
+ case float f:
+ writer.WriteNumberValue(f);
+ return;
+ // Homogeneous OTel arrays. Iterate with the concrete element
type so each
+ // scalar takes the native-JSON path rather than the fallback.
+ case string?[] ss:
+ WriteArray(writer, ss);
+ return;
+ case bool[] bs:
+ WriteArray(writer, bs);
+ return;
+ case long[] ls:
+ WriteArray(writer, ls);
+ return;
+ case int[] iarr:
+ WriteArray(writer, iarr);
+ return;
+ case double[] ds:
+ WriteArray(writer, ds);
+ return;
+ // Generic enumerable fallback (covers ImmutableArray,
List<T>, etc.) —
+ // each element is dispatched recursively.
+ case IEnumerable enumerable:
+ writer.WriteStartArray();
+ foreach (object? item in enumerable)
+ {
+ WriteValue(writer, item);
+ }
+ writer.WriteEndArray();
+ return;
+ // Non-OTel types: invariant-culture string. This keeps the
output
+ // portable across locales for things like DateTime or custom
structs.
+ case IFormattable formattable:
+ writer.WriteStringValue(formattable.ToString(null,
CultureInfo.InvariantCulture));
+ return;
Review Comment:
`OtelAttributeWriter` claims that non-OTel values “fall through to an
invariant-culture string”, but the `IEnumerable` arm runs before the
`IFormattable`/default fallback. This means values like `byte[]` (and
dictionaries or other enumerables) will be serialized as JSON arrays rather
than the documented invariant-string fallback. If that’s not intended, add
explicit handling for `byte[]`/`IDictionary` before `IEnumerable`, or restrict
the enumerable path to the specific homogeneous OTel element types.
##########
csharp/src/Apache.Arrow.Adbc/Apache.Arrow.Adbc.csproj:
##########
@@ -1,10 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
+ <TargetFrameworks>netstandard2.0;net8.0;net10.0</TargetFrameworks>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<PackageReadmeFile>readme.md</PackageReadmeFile>
</PropertyGroup>
+
+ <PropertyGroup Condition="'$(TargetFramework)' == 'net10.0'">
+ <IsAotCompatible>true</IsAotCompatible>
+ </PropertyGroup>
Review Comment:
`net10.0` was added to `TargetFrameworks`, but the repo CI installs .NET SDK
8.x (see `.github/workflows/csharp.yml`). Building with the .NET 8 SDK will
fail with NETSDK1045 when targeting `net10.0`. Either update CI/tooling to use
a .NET 10 SDK, or gate inclusion of `net10.0` behind an SDK-version/MSBuild
condition so default builds (and CI) can still run on .NET 8.
##########
csharp/src/Telemetry/Traces/Listeners/Apache.Arrow.Adbc.Telemetry.Traces.Listeners.csproj:
##########
@@ -1,10 +1,14 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
- <TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>
+ <TargetFrameworks>netstandard2.0;net8.0;net10.0</TargetFrameworks>
<Nullable>enable</Nullable>
</PropertyGroup>
+ <PropertyGroup Condition="'$(TargetFramework)' == 'net10.0'">
+ <IsAotCompatible>true</IsAotCompatible>
+ </PropertyGroup>
Review Comment:
`net10.0` was added to `TargetFrameworks`, but the repo CI installs .NET SDK
8.x (see `.github/workflows/csharp.yml`). Building with the .NET 8 SDK will
fail when it encounters the `net10.0` target. Either update CI/tooling to use a
.NET 10 SDK, or conditionally add `net10.0` only when a compatible SDK is
present.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
