-
Notifications
You must be signed in to change notification settings - Fork 148
feat(csharp/src): Add support for adding and configuring OTel exporters #2949
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
feat(csharp/src): Add support for adding and configuring OTel exporters #2949
Conversation
@CurtHagenlocher - Just a reminder to see if you can review this separate PR to handle exporting trace activity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've left some initial comments but will return to reviewing this later.
csharp/src/Telemetry/Traces/Exporters/Apache.Arrow.Adbc.Telemetry.Traces.Exporters.csproj
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporter.cs
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs
Outdated
Show resolved
Hide resolved
private static IOrderedEnumerable<FileInfo> GetTracingFiles(DirectoryInfo tracingDirectory, string searchPattern) | ||
{ | ||
return tracingDirectory | ||
.EnumerateFiles(searchPattern, SearchOption.TopDirectoryOnly) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no way to enumerate the files in an async fashion?
csharp/test/Telemetry/Traces/Exporters/FileExporter/FileExporterTests.cs
Outdated
Show resolved
Hide resolved
csharp/test/Telemetry/Traces/Exporters/FileExporter/FileExporterTests.cs
Outdated
Show resolved
Hide resolved
{ | ||
/// <summary> | ||
/// Provides access to writing trace files, limiting the | ||
/// individual files size and ensuring unique file names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if two instances of this were pointed at the same directory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CurtHagenlocher
I've added a test (and reworked the retry parameters) to support reasonably heavy concurrency. However, I don't think that would be normal, because this class is called by the FileExporter, that ensures there is only once instance for the folder/basefile name combination. So contention would not be likely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/TracingFile.cs
Outdated
Show resolved
Hide resolved
@CurtHagenlocher - Ready for a new review |
@jduo Let me know if you have any comments or suggestions. Thanks. |
@CurtHagenlocher Checking to see if you have some time to review this PR. Thanks. |
@CurtHagenlocher - anything else needed on this? It would be nice to have support for outputting the BigQuery logs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! What do you think about making this be a standalone executable? It could still be referenced as an assembly and loaded into another process, but that would also be let it used directly.
csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporterOptions.cs
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/FileExporterOptions.cs
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/TracingFile.cs
Outdated
Show resolved
Hide resolved
{ | ||
/// <summary> | ||
/// Provides access to writing trace files, limiting the | ||
/// individual files size and ensuring unique file names. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
csharp/src/Telemetry/Traces/Exporters/FileExporter/SerializableActivity.cs
Show resolved
Hide resolved
I'm afraid I don't understand how a standalone executable would work. Would it be a sort of CLI tool to load and arbitrary driver and run queries? Could you clarify this question? Thanks. |
I've update the file naming to include the process ID. This should allow multiple processes to create files independently of each other without having to compete for the same trace file in the same folder. Let me know if you still have concerns about the concurrency or efficiency of the file generation/re-use design. Thanks. |
I'm assuming that the collector doesn't need to run in the same process as the producer. If that's not true, then this suggestion is pointless. But if it is true, then a collector could be started manually while something was happening and just record traces during that time before being stopped manually again. I may be improperly projecting an internal |
To the best of my knowledge, The |
@CurtHagenlocher P.S.: Thanks for taking the time to review this PR. Much appreciated! |
Adds a new project and tests to support adding OpenTelemetry exporters (TracerProvider).
FileExporter
implementation.ExportersBuilder
thatNote to reviewer:
ExportersBuilder
is a convenience class - let me know if you think it is useful.