Oracle OpenTelemetry support

[cmdkeen]

Closes #4072 by updating to the new ODP.Net version and adding OTel support via Oracle.ManagedDataAccess.OpenTelemetry. As mentioned in the issue this will need a new package and bumped version adding to the NuGet feeds.

Used the tracing settings taken from the Oracle sample to ensure that EF Core SQL text shows is provided to the traces.

Added an EndToEnd test using the Npgsql one as a base.

Microsoft Reviewers: Open in CodeFlow

[cmdkeen]

@mitchdenny or @danmoseley are you able to take a look at this?

[mitchdenny]

Adding @eerhardt for the Otel side of things.

[mitchdenny]

Importing the Otel package to internal feed so we can build this.

[mitchdenny]

Need to update the manifest.

[eerhardt]

This might be a silly question, but why aren't these set to true by default in AddOracleDataProviderInstrumentation? Are there other properties that should be set?

[cmdkeen]

One for the package authors as to why they're not the defaults. I did try originally with just an empty AddOracleDataProviderInstrumentation() constructor call but then didn't see any database calls being traced from the E2E playground. This is the same set of properties as used in the Oracle OTel samples.

Potentially it is because the Oracle OTel library isn't specifically aimed at EF so it might be the way that the Oracle EF package is setting the SQL statement, but hard to tell as it isn't OSS.

[eerhardt]

Do you know any of the authors? Can you tag them here?

[cmdkeen]

Can't see Alex Keh on here, have raised oracle/dotnet-db-samples#378 to ask

[alexkeh]

@eerhardt @cmdkeen Oracle ODP.NET sets these properties to false by default as a security precaution. We didn't want developers to incorporate the ODP.NET OpenTelemetry and then inadvertently publish sensitive DB info into the trace. We want to make sure there was an explicit intent to do so.

Are other DB OpenTelemetry providers enabling these settings by default? I forget whether SqlClient has the options class members automatically enabled (and I couldn't locate the API doc).

[cmdkeen]

Hmm - I'm no expert, from looking at the playground output the Postgres and MySQL ones do emit at least some of this data (SQL command, passwordless connection string and db info) by default whereas it looks like the SqlServer one does not.
From my perspective having that level of detail is incredibly helpful. I can see at least four options:

1. Turning on this detail by default, mentioning in the docs
2. Switch back to the library defaults, allow fine grained control of the instrumentation options via adding a OracleDataProviderInstrumentationOptions property to OracleEntityFrameworkCoreSettings
3. Switch back to the library defaults, add a "DetailedTracing" property to OracleEntityFrameworkCoreSettings that sets these options with a suitable comment
4. Switch back to the library defaults - not sure if you can change the options easily, otherwise users might need to disable tracing and manually add

I can't see any other examples of tracing configuration being passed through the constructors, and appreciate the extensions are trying to set up sensible defaults. My preference is for either 1 or 3 from above, but happy to take a steer.

[eerhardt]

The intent of .NET Aspire components is to codify best practices. So developers will fall into a "pit of success" with using the defaults.

I don't think we should be enabling any setting by default if there are security concerns with that setting being on.

[eerhardt]

Can you add tests for the tracing. They need to be declared at each component level. For example:

aspire/tests/Aspire.Npgsql.EntityFrameworkCore.PostgreSQL.Tests/ConformanceTests.cs

Lines 134 to 137 in 9fab2f6

	[RequiresDockerFact]
	public void TracingEnablesTheRightActivitySource()
	=> RemoteExecutor.Invoke(static connectionStringToUse => RunWithConnectionString(connectionStringToUse, obj => obj.ActivitySourceTest(key: null)),
	ConnectionString).Dispose();

[cmdkeen]

Will do

[cmdkeen]

From what I can see this will require adding the TestContainers.Oracle nuget package to the internal package feed as #3063 hasn't covered Oracle yet. Happy to give that a go.

[cmdkeen]

It also seems that the current .Net Testcontainers is using the XE image, there isn't the equivalent of the Java one having the newer Oracle free module. It also locks some of the config behind private methods that would swapping out the image to support the same image as Hosting uses. A bit of pondering required on the best approach.

[cmdkeen]

Added and also enabled Testcontainers for Oracle as part of it. Turns out the base Testcontainers instance works fine.

[cmdkeen]

Importing the Otel package to internal feed so we can build this.

@mitchdenny or @eerhardt are you able to add Testcontainers.Oracle to the internal feed as well to get this compiling again.

[mitchdenny]

@mitchdenny or @eerharor @eerhardt are you able to add Testcontainers.Oracle to the internal feed as well to get this compiling again.

Importing that package now. Should be there in 30 mins.

[cmdkeen]

@eerhardt @sebastienros would you be able to take a look with the failing tests? I've got them working on my machine, and made some progress on working out what's needed on the them running on the build agent (adding the Helix reference helped) but it looks like the ones that use Docker are still all failing.
I did adjust the TriggerActivity to try and be obvious about connection issues to the database, but it feels like this is something else.

[sebastienros]

@radical The Helix tests are failing on things related to this PR, can you sport something wrong with how the new tests are written?

[radical]

Looking at the test results the first test fails with:

Oracle might be taking a longer time to start up, or failing at some point? I would try to add some waits to confirm that the db is up, and accessible. This might be helpful too.

Also, there is #3161 but it doesn't use testcontainers, and instead depends on the AppHost starting the container.

[eerhardt]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[eerhardt]

This doesn't look correct. Did you just copy-paste this?

[cmdkeen]

Yes, and you're right definitely not accurate. Do I need to generate one or can I just delete the file?

[mitchdenny]

Regenerate it.

[eerhardt]

It looks like we aren't getting the database icon to show up in the dashboard:

compare that to SqlServerEndToEnd:

The reason appears to be because there is no peer.service attribute/tag on the span:

vs.

@JamesNK - is there a reason why the span has to have peer.service for the DB icon to show up? Can't it having db.system be enough?