CurtHagenlocher commented on code in PR #3315:
URL: https://github.com/apache/arrow-adbc/pull/3315#discussion_r2373128856
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2Connection.cs:
##########
@@ -294,6 +301,31 @@ internal HiveServer2Connection(IReadOnlyDictionary<string,
string> properties)
}
}
+ private void TryInitTracerProvider(out FileActivityListener?
fileActivityListener)
+ {
+ Properties.TryGetValue(ListenersOptions.Exporter, out string?
exporterOption);
+ // This listener will only listen for activity from this specific
connection instance.
+ bool shouldListenTo(ActivitySource source) => source.Tags?.Any(t
=> t.Key == _traceInstanceId) == true;
Review Comment:
So we'll potentially have to enumerate all the tags to see if this is the
right listener? There's not a more efficient way to do this?
Consider using a pointer comparison instead i.e.
```suggestion
bool shouldListenTo(ActivitySource source) => source.Tags?.Any(t
=> object.ReferenceEquals(t.Key, _traceInstanceId)) == true;
```
##########
csharp/src/Telemetry/Traces/Exporters/ExportersBuilder.cs:
##########
@@ -94,32 +94,63 @@ public static Builder Build(string sourceName, string?
sourceVersion = default,
out string? exporterName,
string environmentName = ExportersOptions.Environment.Exporter)
{
- TracerProvider? tracerProvider = null;
- exporterName = null;
-
- if (string.IsNullOrWhiteSpace(exporterOption))
+ if (TryActivate(exporterOption, out exporterName, out
TracerProvider? tracerProvider, environmentName))
{
- // Fall back to check the environment variable
- exporterOption =
Environment.GetEnvironmentVariable(environmentName);
+ return tracerProvider;
}
- if (string.IsNullOrWhiteSpace(exporterOption))
+ if (!string.IsNullOrEmpty(exporterName) && exporterName !=
ExportersOptions.Exporters.None)
{
- // Neither option or environment variable is set - no tracer
provider will be activated.
- return null;
+ // Requested option has not been added via the builder
+ throw AdbcException.NotImplemented($"Exporter option
'{exporterName}' is not implemented.");
}
+ return null;
+ }
+
+ /// <summary>
+ /// Tries to activate an exporter based on the dictionary of <see
cref="TracerProvider"/> factories.
+ /// </summary>
+ /// <param name="exporterOption">The value (name) of the exporter
option, typically passed as option <see
cref="ExportersOptions.Exporter"/>.</param>
+ /// <param name="exporterName">The actual exporter name when
successfully activated.</param>
+ /// <param name="tracerProvider">A non-null <see
cref="TracerProvider"/> when successfully activated. Returns null if not
successful. Note: this object must be explicitly disposed when no longer
necessary.</param>
+ /// <param name="environmentName">The (optional) name of the
environment variable to test for the exporter name. Default: <see
cref="ExportersOptions.Environment.Exporter"/></param>
+ /// <returns>Returns true if the exporter was successfully activated.
Returns false, otherwise.</returns>
+ public bool TryActivate(
+ string? exporterOption,
+ out string? exporterName,
+ out TracerProvider? tracerProvider,
+ string environmentName = ExportersOptions.Environment.Exporter)
+ {
+ tracerProvider = null;
+ exporterName = null;
- if (!_tracerProviderFactories.TryGetValue(exporterOption!, out
Func<string, string?, TracerProvider?>? factory))
+ if (!TryGetExporterName(exporterOption, environmentName, out
exporterName)
+ || !_tracerProviderFactories.TryGetValue(exporterName!, out
Func<string, string?, TracerProvider?>? factory))
{
- // Requested option has not been added via the builder
- throw AdbcException.NotImplemented($"Exporter option
'{exporterOption}' is not implemented.");
+ return false;
}
tracerProvider = factory.Invoke(_sourceName, _sourceVersion);
- if (tracerProvider != null)
+ if (tracerProvider == null)
{
- exporterName = exporterOption;
+ return false;
}
- return tracerProvider;
+
+ return true;
+ }
+
+ /// <summary>
+ /// Determines whether the specified exporter option would activate an
exporter.
+ /// </summary>
+ /// <param name="exporterOption">The value (name) of the exporter
option, typically passed as option <see
cref="ExportersOptions.Exporter"/>.</param>
+ /// <param name="exporterName">The actual exporter name when
successfully activated.</param>
+ /// <returns></returns>
+ public bool WouldActivate(string? exporterOption, string
environmentName = ExportersOptions.Environment.Exporter)
Review Comment:
It doesn't appear that this is currently used.
##########
csharp/src/Drivers/Apache/Hive2/README.md:
##########
@@ -149,3 +149,36 @@ The Collector can be configure to receive trace messages
from the driver and exp
Ensure to set the [environment
variable](https://opentelemetry.io/docs/specs/otel/protocol/exporter/)
`OTEL_EXPORTER_OTLP_INSECURE` to `true`, in this scenario.
Ensure to follow [Collector configuration best
practices](https://opentelemetry.io/docs/security/config-best-practices/).
+
+## Tracing
+
+### Tracing Exporters
+
+To enable tracing messages to be observed, a tracing exporter needs to be
activated.
+Use either the environment variable `OTEL_TRACES_EXPORTER` or the parameter
`adbc.traces.exporter` to select one of the
+supported exporters. The parameter has precedence over the environment
variable.
+
+The following exporters are supported:
+
+| Exporter | Description |
+| --- | --- |
+| `adbcfile` | Exports traces to rotating files in a folder. |
+
+Note: _The first connection to activate tracing will enable tracing for
+any later connections that are created in that process._ (This behavior may
change in future implementations.)
Review Comment:
The documentation should probably mention that the connection option needs
to be set *before* the connection is initialized.
##########
csharp/src/Telemetry/Traces/Listeners/FileListener/TracingFile.cs:
##########
@@ -122,6 +126,8 @@ private async Task WriteSingleLineAsync(Stream stream)
await OpenNewTracingFileAsync().ConfigureAwait(false);
}
await stream.CopyToAsync(_currentFileStream).ConfigureAwait(false);
+ // In case of a crash, flush to disk.
Review Comment:
```suggestion
// Flush for robustness to crashing
```
(I found the original wording a little confusing in that the second clause
appeared to be logically dependent on the first.)
##########
csharp/src/Drivers/Apache/Hive2/HiveServer2Connection.cs:
##########
@@ -294,6 +301,31 @@ internal HiveServer2Connection(IReadOnlyDictionary<string,
string> properties)
}
}
+ private void TryInitTracerProvider(out FileActivityListener?
fileActivityListener)
+ {
+ Properties.TryGetValue(ListenersOptions.Exporter, out string?
exporterOption);
+ // This listener will only listen for activity from this specific
connection instance.
+ bool shouldListenTo(ActivitySource source) => source.Tags?.Any(t
=> t.Key == _traceInstanceId) == true;
+ FileActivityListener.TryActivateFileListener(AssemblyName,
exporterOption, out fileActivityListener, shouldListenTo: shouldListenTo);
Review Comment:
Since this method is called `TryInit` should it return the result of calling
`TryActivate`?
##########
csharp/test/Telemetry/Traces/Listeners/FileListener/FileActivityListenerTests.cs:
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.Diagnostics;
+using Apache.Arrow.Adbc.Telemetry.Traces.Listeners;
+using Apache.Arrow.Adbc.Telemetry.Traces.Listeners.FileListener;
+using Apache.Arrow.Adbc.Tracing;
+using Apache.Arrow.Ipc;
+
+namespace Apache.Arrow.Adbc.Tests.Telemetry.Traces.Listeners.FileListener
+{
+ public class FileActivityListenerTests
+ {
+ private const string TraceLocation = "adbc.trace.location";
+
+ [Theory]
+ [InlineData(null, false)]
+ [InlineData("", false)]
+ [InlineData(" ", false)]
+ [InlineData(ListenersOptions.Exporters.None, false)]
+ [InlineData(ListenersOptions.Exporters.AdbcFile, true)]
+ public void TestTryActivateFileListener(string? exporterOption, bool
expected)
+ {
+ Assert.Equal(expected,
FileActivityListener.TryActivateFileListener("TestSource", exporterOption, out
FileActivityListener? listener));
+ Assert.True(expected == (listener != null));
+ listener?.Dispose();
+ }
+
+ [Fact]
+ public async Task CanTraceConcurrentConnections()
+ {
+ const int numConnections = 5;
+ const int numActivitiesPerConnection = 1000;
+ string folderLocation =
Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.LocalApplicationData),
TracingFileTests.NewName());
+
+ try
+ {
+ TestConnection[] connections = new
TestConnection[numConnections];
+ for (int i = 0; i < numConnections; i++)
+ {
+ connections[i] = new TestConnection(new Dictionary<string,
string>
+ {
+ { ListenersOptions.Exporter,
ListenersOptions.Exporters.AdbcFile },
+ { TraceLocation, folderLocation },
+ });
Review Comment:
nit: fix indentation
--
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
