-
Notifications
You must be signed in to change notification settings - Fork 98
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
README update: Add Oracle .NET OpenTelemetry provider #3192
Comments
@alexkeh, The ultimate solution for introducing native OTel support for traces for library is to just call I have checked what is doing From Oracle.ManagedDataAccess
and
From
Please consider following changes:
Based on your answers, we can update documentation/add support for Oracle db in AutomaticInstrumentation. BTW Based on this: https://opentelemetry.devstats.cncf.io/d/5/companies-table?orgId=1&var-period_name=Last%20month&var-metric=contributions Oracle is pretty important contributor to OpenTelemetry. You can check with @marcalff how to achieve contributor status. |
Thanks for the detailed feedback @Kielek. I will discuss these recommendations with the Oracle .NET dev team. |
@alexkeh, do you have any plan to address this feedback? Or you keep current way for the implementation? |
@Kielek Due to the holidays and different team members being on vacation, including myself, I haven't had a chance to discuss these recommendations with the full dev team yet. |
@Kielek
When we developed ODP.NET OpenTelemetry we emulated API behavior in other OpenTelemetry providers, such as Microsoft SqlClient and Npgsql. Those providers don't appear to be following this recommendation. Yet, they are supported by autoinstrumentation. Is there a reason why ODP.NET specifically needs to follow this recommendation?
We plan to change the name to "ODP" for the ODP.NET Core provider, which runs in .NET (Core) and "ODPM" for the managed ODP.NET provider, which runs in .NET Framework.
We're planning to add these changes.
We're planning to add these changes.
Ok. |
Microsoft SqlClient instrumentation is not the best way to follow. It was developed before the OpenTelemetry was popular and it internally utilize DiagnosticListener to translate events to the Activities. It requires as to reference additional library to consume these functionality. I suppose that, int future, new versions of the libraries will be rewritten to provide native support for OTel by the ActivitySource. Npqsql is a good example how to develop implementation for OTel native support. Npgsql-like design allow us to enable the support for the any library without needs to reference any additional library (which is bad, because brings dependencies which could make a conflicts in runtime) and without using reflection (as forcing to enable
My recommendation for both, it should be "Oracle.ManagedDataAccess". The language, framework can be deducted based on the resource attributes attached to the traces. See OpenTelemetry.ResourceDetectors.ProcessRuntime. If your code is somehow publicly available, I can find some time to provide PR/code changes with the discussed feedback. |
@Kielek Is the reason you advise the removal of the internal property |
It is how the ActivitySource API is designed. Everything from the client perspective, should be to call "AddSource" to the OTel (or any other activity source listener). There should be no need to look into any internal details of the library. As a reference please check:
And the MS official documentation: To sum up, Any library natively supporting should call var activity = activitySource.StartActivity(). Then you should check
Please let me know if it answers your doubts. |
|
@alexkeh, do you have any progress in Oracle instrumentation? |
@Kielek We plan to release a new ODP.NET OpenTelemetry version this month. It will include some of the changes you recommend. |
@Kielek Oracle released a new ODP.NET OpenTelemetry provider this week. We've implemented all of the suggestions you made in December. Thanks for evaluating our previous release and supplying the detailed feedback. Is there anything else recommended to enable automatic instrumentation? |
Hi @Kielek We wanted some clarification from you regarding the following statement that you mentioned in one of your posts above.
From what we understand, what we have done for ODP.NET’s support for open telemetry is manual instrumentation as we have manually instrumented our code using System.Diagnostics API’s. Here is a link which describes about open telemetry automatic instrumentation. From what is mentioned in above link, I don’t think what ODP.NET is done can be classified as automatic instrumentation. Please could you clarify if my understanding is correct? |
@alexkeh, @pfdsilva, based on short review I suppose that you can check your application with 1.4.0 release. You can set I have a plan to add support for these two ActvitySources by default. It probably happen after March 25, next week I will have very limited time to work on this. One more important change in db semantic convention open-telemetry/semantic-conventions#769 |
@Kielek I have a test app that uses ODP.NET's ODPC 23c provider. I was able to enable ODPC's open telemetry traces by doing the following.
|
@pfdsilva, @alexkeh, The plan is to keep it as a draft until you release stable version of packages. Additional feedback after testing:
It is great to see such progress in Oracle instrumentation. It is pretty high on our users whish list! |
@Kielek
|
Ref 9746fa3 It allows to configure Oracle option by environmental variable. Technically we can have it here, but it should be easier (for us) to read this values eg in constructor or by IConfiguration with defaulting to Env. Vars.: Something like:
|
@Kielek
Does any of the other open telemetry providers support something similar?
|
@Kielek We plan to release a stable version of ODP.NET OpenTelemetry by the end of the week of April 29. I'll let you know the day it happens. |
@pfdsilva, Versioning - almost all instrumetnation packages in https://github.com/open-telemetry/opentelemetry-dotnet-contrib and https://github.com/open-telemetry/opentelemetry-dotnet report library version by new ActivitySource |
@Kielek Oracle has released its Oracle.OpenTelemetry provider to production on NuGet Gallery. The version is 23.4. Please commit your changes to the .NET OpenTelemetry libraries and update the README. Thanks! |
Oracle recently released its .NET OpenTelemetry provider that works with both Oracle.ManagedDataAccess.Core (ODP.NET Core) and Oracle.ManagedDataAccess (managed ODP.NET). Can you update the Instrumentation Libraries README Databases table with the following or something similar?
I realize I could file a PR with the README change. However, going through the EasyCLA from a corporate standpoint is a big unknown in terms of time commitment and effort to complete when the change is straightforward and the update is from public info.
Thanks!
The text was updated successfully, but these errors were encountered: