JasperFx / marten

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

Add LogBeforeExecute method to IMatenSessionLogger #1678

Closed nazarovsa closed 3 years ago

nazarovsa commented 3 years ago

Hello. Is it possible to add LogBeforeExecute method to IMartenSessionLogger which will be executed before executing NpsqlCommand's? In my case I want to use a custom session logger to wrap db queries execution into Elastic APM Span, but now I can't initialize Span at right place. I want to initialize Elastic Span with NpgsqlCommand.CommandText, etc.

Sample of my custom logger:

    private class MartenApmLogger : IMartenSessionLogger
    {
            private ISpan _span;

            public void LogBeforeExecute(NpgsqlCommand command)
            {
                var transaction = Elastic.Apm.Agent.Tracer.CurrentTransaction;
                if (transaction != null)
                {
                    _span = transaction
                        .StartSpan(command.CommandText, ApiConstants.TypeDb, Marten);
                }
            }

            public void LogSuccess(NpgsqlCommand command)
            {
                _span?.End();
            }

            public void LogFailure(NpgsqlCommand command, Exception ex)
            {
                _span?.CaptureException(ex);
            }

            public void RecordSavedChanges(IDocumentSession session, IChangeSet commit)
            {
            }
    }

I think this changes can be good for similar behaviors. At first look, changes required only at IMartenSessionLogger interface and ManagedConnection class. (and base realizations of IMartenSessionLogger ofc.)

If you will approve my suggestion, I'm ready to write some code.

oskardudycz commented 3 years ago

I'm fine with that. @jeremydmiller @mysticmind thoughts?

jeremydmiller commented 3 years ago

@babuannamalai @oskardudycz I thought about this one a little bit, and I'm not thinking of a better way per se. The LogBeforeExecute() needs to have an argument for IDocumentSession though so you can pick up the correlation/causation/header metadata information in the logging. Since it's V4, maybe add the IDocumentSession to all the other methods as well.

See this comment as well: https://github.com/JasperFx/marten/issues/1548#issuecomment-741689828

Before we release V4 for real, I think I'd like to take a little detour and add full Open Telemetry support to Jasper, then do some kind of simplistic sample app that proves we can do open telemetry trading from HTTP endpoint all the way through to Marten itself just to make sure we've got everything we need.