Skip to content
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

OpenTelemetry tracing and metrics #1196

Open
willibrandon opened this issue Oct 12, 2024 · 28 comments
Open

OpenTelemetry tracing and metrics #1196

willibrandon opened this issue Oct 12, 2024 · 28 comments

Comments

@willibrandon
Copy link
Contributor

Hello, thanks for the great work.

I am working on .NET Aspire integrations for Firebird and wonder if the .NET data provider is instrumented with OpenTelemetry tracing and metrics?

@cincuranet
Copy link
Member

Yes. It is on my radar.

@willibrandon
Copy link
Contributor Author

I'm able to collect EF Core traces using EntityFrameworkCore Instrumentation for OpenTelemetry .NET.

2024-10-19T08:59:05.6990000 2024-10-19T01:59:05.6986664 info: Microsoft.EntityFrameworkCore.Database.Command[20101]
2024-10-19T08:59:05.6990000       Executed DbCommand (24ms) [Parameters=[], CommandType='Text', CommandTimeout='0']
2024-10-19T08:59:05.7000000       SELECT "c"."Id", "c"."Brand"
2024-10-19T08:59:05.7000000       FROM "CatalogBrands" AS "c"
2024-10-19T08:59:05.7010000 Activity.TraceId:            3bc7ce1e7495b4f44f636917ef5fb63e
2024-10-19T08:59:05.7010000 Activity.SpanId:             273cf93473216c11
2024-10-19T08:59:05.7010000 Activity.TraceFlags:         Recorded
2024-10-19T08:59:05.7010000 Activity.ParentSpanId:       1e296c6cd31eb0f1
2024-10-19T08:59:05.7020000 Activity.ActivitySourceName: OpenTelemetry.Instrumentation.EntityFrameworkCore
2024-10-19T08:59:05.7020000 Activity.ActivitySourceVersion: 1.0.0-beta.12
2024-10-19T08:59:05.7030000 Activity.DisplayName:        /var/lib/firebird/data/firebirdDb
2024-10-19T08:59:05.7030000 Activity.Kind:               Client
2024-10-19T08:59:05.7040000 Activity.StartTime:          2024-10-19T08:59:05.6726693Z
2024-10-19T08:59:05.7040000 Activity.Duration:           00:00:00.0280476
2024-10-19T08:59:05.7040000 Activity.Tags:
2024-10-19T08:59:05.7040000     db.system: firebird
2024-10-19T08:59:05.7050000     db.name: /var/lib/firebird/data/firebirdDb
2024-10-19T08:59:05.7050000     peer.service: localhost
2024-10-19T08:59:05.7060000 Resource associated with Activity:
2024-10-19T08:59:05.7060000     service.name: apiservice
2024-10-19T08:59:05.7060000     service.instance.id: k4n16oeao8
2024-10-19T08:59:05.7080000     telemetry.sdk.name: opentelemetry
2024-10-19T08:59:05.7080000     telemetry.sdk.language: dotnet
2024-10-19T08:59:05.7080000     telemetry.sdk.version: 1.9.0

@willibrandon
Copy link
Contributor Author

Here is an example of the EF Core trace in the Aspire dashboard.

AspireEFCoreTrace

And the console log.

AspireEFCoreTraceConsoleLog

@fdcastel
Copy link
Contributor

Has any work been done in this area already?

I'm interested in this feature and would be happy to contribute with some PRs.

@fdcastel
Copy link
Contributor

A proposed plan of action could be:

  • Replace all static classes from FirebirdSql.Data.Logging with corresponding components from the .NET ILogger API;
  • Create an ActivitySource (static) for FirebirdSql.Data;
  • Integrate tracing instrumentation by adding Activities throughout the code;
  • (Optional) Add Metrics?

This would only utilize components from .NET runtime, without adding any extra dependencies.

@willibrandon
Copy link
Contributor Author

Has any work been done in this area already?

Just some research looking into what exists now, and what possible improvements could be made to enhance the observability of the system.

@willibrandon
Copy link
Contributor Author

@fdcastel - I just recognized your username and thank you very much for the docker images!

@cincuranet
Copy link
Member

Has any work been done in this area already?

Not really. I played with replacing the current implementation ILogger, but nothing committed.

Metrics is definitely something worth discussing and adding. Spans/tracing makes probably sense for command execution. Both metrics and spans need to have sensible attributes.

@fdcastel
Copy link
Contributor

Has any work been done in this area already?

Not really. I played with replacing the current implementation ILogger, but nothing committed.

  • If you wish, I can continue what you started in a separate fork;
  • Or start from zero, if you are not satisfied with the current state of what you have today;
  • Or we can just sit and wait for you to do everything 😉

Metrics is definitely something worth discussing and adding. Spans/tracing makes probably sense for command execution. Both metrics and spans need to have sensible attributes.

Agreed. About this, Microsoft.Data.SqlClient is probably a good benchmark.

@cincuranet
Copy link
Member

You can start with logging from scratch.

@fdcastel
Copy link
Contributor

Quick question:

Fork from here or https://github.com/cincuranet/FirebirdSql.Data.FirebirdClient ?

@cincuranet
Copy link
Member

Here.

@willibrandon
Copy link
Contributor Author

Here are the common attributes for client metrics.
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-metrics.md

@fdcastel
Copy link
Contributor

Thanks @willibrandon. I will start to work on it now.

@fdcastel
Copy link
Contributor

I just pushed into #1200 the last "pillar of observability" that was missing (metrics).

I chose not to implement all OpenTelemetry suggested metrics because, frankly, some are really questionable. But it is a good start.

Have a great weekend! 😄

@willibrandon
Copy link
Contributor Author

@fdcastel - Taking your changes for a run 🏃

image

image

@willibrandon
Copy link
Contributor Author

I can foresee extension method APIs for MeterProviderBuilder and TraceProviderBuilder that could be created in order to assist in subscribing to FirebirdSql.Data OpenTelemetry tracing and metrics.

/// <summary>
///  Extension method for setting up FirebirdSql.Data OpenTelemetry tracing.
/// </summary>
public static class TracerProviderBuilderExtensions
{
	/// <summary>
	///  Subscribes to FirebirdSql.Data activity source to enable OpenTelemetry tracing.
	/// </summary>
	public static TracerProviderBuilder AddFirebird(
		this TracerProviderBuilder builder)
		=> builder.AddSource("FirebirdSql.Data");
}

/// <summary>
///  Extension method for setting up FirebirdSql.Data OpenTelemetry metrics.
/// </summary>
public static class MeterProviderBuilderExtensions
{
	/// <summary>
	///  Subscribes to FirebirdSql.Data meter to enable OpenTelemetry metrics.
	/// </summary>
	public static MeterProviderBuilder AddFirebird(
		this MeterProviderBuilder builder)
		=> builder.AddMeter("FirebirdSql.Data");
}

@fdcastel
Copy link
Contributor

Regarding the extension methods, I believe the best course of action would be to create a new package OpenTelemetry.Instrumentation.FirebirdSql, in line with OpenTelemetry guidelines.

This would allow the FirebirdSql.Data.FirebirdClient project to remain free of any OpenTelemetry packages dependencies.

However, I've deferred this discussion once the instrumentation code is more refined.

So, please give it a try in real scenarios and bring us your feedback. So far, I’ve only tested it within the Aspire Dashboard (and OpenObserve) on my end with some basic use cases.

@willibrandon
Copy link
Contributor Author

Will do. Initially I was motivated because I saw a gap with Aspire integrations, but now I see this could be more useful than that. Where I work we are responsible for a large and growing number of Firebird databases running in retail stores across the United States. There, I support a small team of database administrators who I know would appreciate additional metrics, logs, and traces to help them analyze our software's performance and behavior. I will try and get their feedback.

@fdcastel
Copy link
Contributor

Please note that Aspire Dashboard never intended to be a production system. Is more a "developer helper"
(or, in Microsoft words, "purpose-built to enhance the development experience").

If you want something battle tested, Seq is probably a good option. Though, of course, there are several other professional options available.

@fdcastel
Copy link
Contributor

And, on a completely unrelated note, it’s great to hear that Firebird is being utilized in large-scale operations in the U.S.! 🚀

@willibrandon
Copy link
Contributor Author

First off, thank you for putting together this OpenTelemetry instrumentation proposal! I’ve done some initial testing in real scenarios, and here are a few thoughts and observations:

  1. Tracing

    • Having Activity spans around each command is great—it fits well with how other ADO.NET providers handle telemetry. The use of attributes like db.system and db.query.text (or db.statement) is aligned with OpenTelemetry conventions.
    • I especially appreciate that exceptions are caught and recorded properly in the span. It’s very helpful for diagnosing issues.
  2. Metrics

    • The new histograms (db.client.operation.duration, etc.) and counters (db.client.connection.count, db.client.connection.max) give valuable insights into query performance and connection pool usage. It’s easy to see at a glance when we’re approaching pool limits.
  3. Connection Instrumentation

    • Measuring Open() and OpenAsync() times is really useful. If you ever plan to capture “time waiting for a connection from the pool,” that might be a nice next step.
  4. Performance & Logging

    • Overall, overhead seems minimal when no listeners are attached. Using Microsoft.Extensions.Logging is also a nice modernization, letting us plug in our existing log pipeline.
    • Parameter logging toggles are a great touch—some environments can’t store full param data, so having a quick way to disable that is appreciated.
  5. Potential Small Tweaks

    • You may want to rename a couple of attributes (e.g., db.statement instead of db.query.text) to exactly match OTel specs, but that’s more of a nitpick than a requirement.
    • Sanitizing or truncating large SQL statements or sensitive parameter values might be worth considering in certain scenarios.

Overall, this is a fantastic addition and makes the Firebird ADO.NET experience much more observability-friendly. Thanks again for taking the time to implement these changes—I’m looking forward to continuing to test them out and will share more feedback as I gather more data in production environments.

@fdcastel
Copy link
Contributor

Thank you @willibrandon for the valuable feedback. 👍🏻

I will be away for the next ~10 days but I will surely address your suggestions upon my return.

Meanwhile, if you can, please provide more detailed points you would like to see addressed.

Whenever possible, citing sources. E.g. Where did you get that db.statement is the recommended attribute name? I got db.query.text from here.

@fdcastel
Copy link
Contributor

@cincuranet What about the creation of a new package as discussed here?

@fdcastel
Copy link
Contributor

@cincuranet Additionally, if possible, could you offer guidance on how to measure the 'time spent waiting for a connection from the pool,' as referenced by @willibrandon in the third point?

@willibrandon
Copy link
Contributor Author

@fdcastel Thanks for the update, and I really appreciate you taking time to review this in detail! I realize now that my mention of db.statement came from an older version of the OpenTelemetry specs, where that attribute was commonly used. The latest Database Client Semantic Conventions indeed recommend db.query.text for the full SQL text and potentially db.query.summary for a lower-cardinality representation.

Sorry for any confusion that caused—your reference is spot on. I look forward to refining the implementation with the new guidance once you’re back. In the meantime, if there are any other details you’d like me to clarify or test, just let me know!

@fdcastel
Copy link
Contributor

I realize now that my mention of db.statement came from an older version of the OpenTelemetry specs

You're not alone! 😅 When I was coding, I ended up using an old version of the document for a good chunk of it.

Eventually, I caught my mistake and updated the attributes to match the latest version.

In the meantime, if there are any other details you’d like me to clarify or test, just let me know!

If possible, please share any other additional metrics/attributes you think are relevant for your real-world scenario.

As I said earlier I tried to stick to the specs exactly, but some of them seemed pretty useless. So, I left those out -- at least until someone can show me a solid reason to include them.

@cincuranet
Copy link
Member

@fdcastel

What about the creation of a new package as discussed #1196 (comment)?

We don't need separate package. .NET has all the pieces in the box (Activity, ILogger, ...) and OTEL plugs into (or rather on top) it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants