CurtHagenlocher commented on code in PR #2949: URL: https://github.com/apache/arrow-adbc/pull/2949#discussion_r2277549023
########## csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporterOptions.cs: ########## @@ -0,0 +1,47 @@ +/* + * 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. + */ + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// The options an <see cref="FileExporter"/> uses to trace active to files. Review Comment: ```suggestion /// The options a <see cref="FileExporter"/> uses to trace activity to files. ``` ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporterOptions.cs: ########## @@ -0,0 +1,47 @@ +/* + * 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. + */ + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// The options an <see cref="FileExporter"/> uses to trace active to files. + /// </summary> + public class FileExporterOptions + { + /// <summary> + /// Gets or sets the base file name (typically the tracing source name). + /// Trace files will be created with the following name template: {fileBaseName}-trace-{dateTime}.log + /// </summary> + public string FileBaseName { get; set; } = FileExporter.ApacheArrowAdbcNamespace; + + /// <summary> + /// The full or partial path to a folder where the trace files will be written. + /// If the folder doesn not exist, it will be created. Review Comment: ```suggestion /// If the folder does not exist, it will be created. ``` ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/SerializableActivity.cs: ########## @@ -0,0 +1,192 @@ +/* + * 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.Generic; +using System.Diagnostics; +using System.Linq; +using System.Text.Json.Serialization; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// Simplified version of <see cref="Activity"/> that excludes some properties, etc. + /// </summary> + internal class SerializableActivity + { + [JsonConstructor] Review Comment: As a followup, can we generate the serialization code for this at build time instead of runtime using the source generator? ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporter.cs: ########## @@ -0,0 +1,271 @@ +/* + * 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.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using OpenTelemetry; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + internal class FileExporter : BaseExporter<Activity> + { + internal const long MaxFileSizeKbDefault = 1024; + internal const int MaxTraceFilesDefault = 999; + internal const string ApacheArrowAdbcNamespace = "Apache.Arrow.Adbc"; + private const string TracesFolderName = "Traces"; + + private static readonly string s_tracingLocationDefault = TracingLocationDefault; Review Comment: Consider changing `TracingLocationDefault` to be a method ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs: ########## @@ -0,0 +1,190 @@ +/* + * 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.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// Provides access to writing trace files, limiting the + /// individual files size and ensuring unique file names. + /// </summary> + internal class TracingFile + { + private static readonly string s_defaultTracePath = FileExporter.TracingLocationDefault; + private static readonly Random s_random = new Random(); + private readonly string _fileBaseName; + private readonly DirectoryInfo _tracingDirectory; + private FileInfo? _currentTraceFileInfo; + private readonly long _maxFileSizeKb = FileExporter.MaxFileSizeKbDefault; + private readonly int _maxTraceFiles = FileExporter.MaxTraceFilesDefault; Review Comment: Consider replacing these with properties as they are fixed. e.g. `private int MaxTraceFiles => FileExporter.MaxTraceFiles;` ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs: ########## @@ -0,0 +1,190 @@ +/* + * 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.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// Provides access to writing trace files, limiting the + /// individual files size and ensuring unique file names. + /// </summary> + internal class TracingFile + { + private static readonly string s_defaultTracePath = FileExporter.TracingLocationDefault; + private static readonly Random s_random = new Random(); Review Comment: `System.Random` is not concurrency-safe. If this can be accessed from multiple threads at the same time, it will need to be guarded e.g. by a lock. ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs: ########## @@ -0,0 +1,233 @@ +/* + * 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.Generic; +using System.IO; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + /// <summary> + /// Provides access to writing trace files, limiting the + /// individual files size and ensuring unique file names. Review Comment: So is this part of the contract, then, that two processes which each instantiate this will have to point to different directories? If so, consider adding this detail to readme.md. ########## csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporter.cs: ########## @@ -0,0 +1,271 @@ +/* + * 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.Concurrent; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Text; +using System.Text.Json; +using System.Threading; +using System.Threading.Tasks; +using OpenTelemetry; + +namespace Apache.Arrow.Adbc.Telemetry.Traces.Exporters.FileExporter +{ + internal class FileExporter : BaseExporter<Activity> + { + internal const long MaxFileSizeKbDefault = 1024; + internal const int MaxTraceFilesDefault = 999; + internal const string ApacheArrowAdbcNamespace = "Apache.Arrow.Adbc"; + private const string TracesFolderName = "Traces"; + + private static readonly string s_tracingLocationDefault = TracingLocationDefault; + private static readonly ConcurrentDictionary<string, Lazy<FileExporterInstance>> s_fileExporters = new(); + private static readonly byte[] s_newLine = Encoding.UTF8.GetBytes(Environment.NewLine); + + private readonly TracingFile _tracingFile; + private readonly string _fileBaseName; + private readonly string _tracesDirectoryFullName; + private readonly ConcurrentQueue<Activity> _activityQueue = new(); + private readonly CancellationTokenSource _cancellationTokenSource; + + private bool _disposed = false; + + internal static bool TryCreate(FileExporterOptions options, out FileExporter? fileExporter) + { + return TryCreate( + options.FileBaseName ?? ApacheArrowAdbcNamespace, + options.TraceLocation ?? TracingLocationDefault, Review Comment: ```suggestion options.TraceLocation ?? s_tracingLocationDefault, ``` -- 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: github-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
