JasperFx / marten

.NET Transactional Document DB and Event Store on PostgreSQL
https://martendb.io
MIT License
2.86k stars 456 forks source link

Open Telemetry Support #2909

Closed jeremydmiller closed 6 months ago

jeremydmiller commented 10 months ago

This is a placeholder for a conversation about what this really means for Marten.

Some initial thoughts about what this could be:

SeanFarrow commented 10 months ago

I wonder whether it makes sense to have a separate package Marten.OpenTelemetry that contains the IDocumentSessionListener implementations for points 2 and 3 above. This could also deal with point 1 as well.

In terms of queries, I agree that two options should be made available, I can either specify I want metrics for all queries or just some of them. If the latter is the case, I would think specifying individual query types would be the best option. Whether this makes the cut for version 7 would depend on how many internal changes would be needed to support this, at least that's how I'm reading it.

I'm happy to help where I can as I have a project that uses Open Telemetry and am seriously looking at martin as an option for data storage.

jeremydmiller commented 10 months ago

@SeanFarrow The only reference you need is System.Diagnostics, and that's pretty lightweight. I don't think this justifies a separate package. But also, I don't think it's a breaking change at all, so might slide

ElanHasson commented 10 months ago

While looking up how to do this with Marten, I found this: https://github.com/JasperFx/marten/pull/2358 Also this: https://event-driven.io/en/set_up_opentelemetry_wtih_event_sourcing_and_marten/

Could serve as a great starting point.

@jeremydmiller you will need references to OpenTelemtry nugets if you're to add extension methods on the types to make integration easier: MeterProviderBuilder, TracerProviderBuilder.

This could be a small package with the extension methods.

SeanFarrow commented 10 months ago

I'd definitely like to take this on.

jeremydmiller commented 9 months ago

@ElanHasson No, you don't. All you need is a reference to System.Diagnostics.DiagnosticSource

@SeanFarrow If you're up for that, we'd take you up on that.

jeremydmiller commented 9 months ago

Ideas

Tracking correlation and causation on all sessions

In the constructor for QuerySession, we could automatically set CausationId and CorrelationId by looking for any current Activity. See this code from Wolverine that does something similar for its MessageBus entry point: https://github.com/JasperFx/wolverine/blob/main/src/Wolverine/Runtime/MessageBus.cs#L16

Doing that gives you the tracing all the way to events or documents saved automatically.

Activity or Span for Executing Queries

    int Execute(NpgsqlCommand cmd);
    Task<int> ExecuteAsync(NpgsqlCommand command, CancellationToken token = new());

    DbDataReader ExecuteReader(NpgsqlCommand command);

    Task<DbDataReader> ExecuteReaderAsync(NpgsqlCommand command,
        CancellationToken token = default);

    DbDataReader ExecuteReader(NpgsqlBatch batch);

    Task<DbDataReader> ExecuteReaderAsync(NpgsqlBatch batch,
        CancellationToken token = default);

    void ExecuteBatchPages(IReadOnlyList<OperationPage> pages, List<Exception> exceptions);
    Task ExecuteBatchPagesAsync(IReadOnlyList<OperationPage> pages,
        List<Exception> exceptions, CancellationToken token);

That'd give you performance numbers based on the span. You can make this opt in so folks that aren't taking advantage of it don't get the little bit of performance hit.

We can catch exceptions and tag activity's as being failed. That might be valuable. Not a replacement for logging, but gives you some metrics

Event Metrics

SeanFarrow commented 9 months ago

I’m definitely up for that. I’ll start with the QuerySession piece in the next few days. How do we want to expose the ability to opt-in to IConnectionLifetime activity tracking? Perhaps something on one of the options classes or the DocumentStore? The tagging system for events makes sense. Do we want to provide an easy way for a user to add the opt-in event tagging support that automatically adds the IDocumentSessionListener implementation? Thanks, Sean.

From: Jeremy D. Miller @.> Sent: Monday, February 12, 2024 9:49 PM To: JasperFx/marten @.> Cc: Sean Farrow @.>; Mention @.> Subject: Re: [JasperFx/marten] Open Telemetry Support (Issue #2909)

Ideas Tracking correlation and causation on all sessions

In the constructor for QuerySession, we could automatically set CausationId and CorrelationId by looking for any current Activity. See this code from Wolverine that does something similar for its MessageBus entry point: https://github.com/JasperFx/wolverine/blob/main/src/Wolverine/Runtime/MessageBus.cs#L16

Doing that gives you the tracing all the way to events or documents saved automatically.

Activity or Span for Executing Queries

That'd give you performance numbers based on the span. You can make this opt in so folks that aren't taking advantage of it don't get the little bit of performance hit.

We can catch exceptions and tag activity's as being failed. That might be valuable. Not a replacement for logging, but gives you some metrics

Event Metrics

— Reply to this email directly, view it on GitHubhttps://github.com/JasperFx/marten/issues/2909#issuecomment-1939645986, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7SU2TH3WIL5E2UJ4OLYTKE6HAVCNFSM6AAAAABB3PRURCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMZZGY2DKOJYGY. You are receiving this because you were mentioned.Message ID: @.***>

jeremydmiller commented 9 months ago

Cool @SeanFarrow!

How do we want to expose the ability to opt-in to IConnectionLifetime activity tracking? Perhaps something on one of the options classes or the DocumentStore?

The configuration on StoreOptions, then the branching code is actually in SessionOptions where it normally creates the IConnectionLifetime

Do we want to provide an easy way for a user to add the opt-in event tagging support that automatically adds the IDocumentSessionListener implementation?

Could definitely add helper methods on StoreOptions that just register the right IDocumentSessionListener

Thanks!

SeanFarrow commented 9 months ago

I'm just starting to work on this now. We can use the current activities root id if it is set for the corelation id, but I was wondering What property of the current activity we want to use as the causation id?

jeremydmiller commented 9 months ago

@SeanFarrow Sorry for the slow response. I say you take the correlation id & causation id from any kind of current activity. Marten is completely passive, so I'd expect us to just use the parent activity, whatever that happens to be.

SeanFarrow commented 9 months ago

OK, I'll start work on this in the next few days.

This may take a bit of time, as I'm dealing with a death in the family.

I'll open a working PR so we can all keep track of what's going on.

jeremydmiller commented 8 months ago

@SeanFarrow Hey Sean, sorry to hear that. Family first. Independent of this, @oskardudycz & I think this won't be a breaking change and can slide to 7.1. Might be the banner change when that hits.

jeremydmiller commented 7 months ago

@SeanFarrow Any update on this one?

SeanFarrow commented 7 months ago

It’s something I’m hoping to pick up again the next week or so. Thanks, Sean.

From: Jeremy D. Miller @.> Sent: Friday, March 22, 2024 1:46 PM To: JasperFx/marten @.> Cc: Sean Farrow @.>; Mention @.> Subject: Re: [JasperFx/marten] Open Telemetry Support (Issue #2909)

@SeanFarrowhttps://github.com/SeanFarrow Any update on this one?

— Reply to this email directly, view it on GitHubhttps://github.com/JasperFx/marten/issues/2909#issuecomment-2015143507, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AALDK7TSBNPZIV35JVW4KA3YZQYZBAVCNFSM6AAAAABB3PRURCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJVGE2DGNJQG4. You are receiving this because you were mentioned.Message ID: @.***>

SeanFarrow commented 7 months ago

@jeremydmiller I'll open a PR early with the connection lifetime stuff in the next few days. I'm then away for the next week, so will pick this back up around the 8th. Do we want to merge things in gradually?

ElanHasson commented 6 months ago

HI @SeanFarrow any updates on this?

SeanFarrow commented 6 months ago

@jeremydmiller I am just looking at the document session listener piece of this, for both appending the number of each event type processed, the number of total events and the number of documents being stored.

Do we want to split this in to two separate listeners? It probably makes sense too do this as people may just be using the document storage piece, the event store piece or both together.

Thanks, Sean.