[FEATURE REQ] Allow advanced customization using `EventProcessorClient` about azure-sdk-for-net HOT 7 OPEN

Thank you for your feedback. Tagging and routing to the team member best able to assist.

from azure-sdk-for-net.

Hi @shlomiassaf. Thank you for reaching out and for your suggestions. The EventProcessorClient is intentionally an opinionated implementation on top of EventProcessor<T> bound to Blob Storage and we have no plans to change that.

All of the core functionality of EventProcessorClient is implemented in its lower-level ancestors, PluggableCheckpointStoreEventProcessor and EventProcessor<T>. EventProcessorClient extends this to add the .NET event handlers, unroll the batch for single dispatch, and the Blob Storage integration. There's nothing that EventProcessorClient does that is not available in the base class hierarchy - which includes the OTel diagnostics spans.

While we are open to providing additional checkpoint stores and specialized processor clients, we have not received enough developer feedback requesting them to justify the overhead of developing and maintaining them. To date, we've released beta implementations for Redis and Table Storage in some languages which have seen very low engagement.

For now, this is not something that we'll be moving forward with.

from azure-sdk-for-net.

Hi @shlomiassaf. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

from azure-sdk-for-net.

@jsquire This is not 100% accurate

In EventProcessorClient there are things required for normal operation that should be there, it is only opinionated towards the Blob implementation.

Let's say I would want to implement my own checkpoint implementation.

I would not be able to scope the checkpoint methods...

    using var scope = ClientDiagnostics.CreateScope(DiagnosticProperty.EventProcessorCheckpointActivityName, ActivityKind.Internal);
            scope.Start();

As it's heavily guarded.

I have no complaints really, as long as i'm able to follow the standards of the SDK and frankly telemetry is important.

I do think the structure should be different, however thats arguable so your way of doing it is great.
But that's just requires some access or additional wrapping to allow the core behavior to remain.

Maybe PluggableCheckpointStoreEventProcessor should have scoping wrapped internally in the checkpoint calls, maybe exposing them? either way it's missing

And don't get me wrong, I believe the external offset approach is much more diverse and powerful, as it allows massive vertical scaling, and to be honest, using Kafka API will expose seamless checkpoints anyway

from azure-sdk-for-net.

/unresolve

from azure-sdk-for-net.

@shlomiassaf : That's a fair point; it does appear that we overlooked the checkpoint diagnostic scope when pulling out the PluggableCheckpointStoreEventProcessor. This may be an intentional choice because checkpointing is an internal activity type and not defined as part of the official OTel messaging spec.

One thing to note is that there's nothing special about ClientDiagnostics - it is simply an internal helper class for working with the System.Diagnostics classes. Any application can participate in activities and extend the context by using the Activity class and related elements.

For now, I'll leave this open and talk this over with our OTel expert when I'm back in the office next week and see if this is something that we should move up.

I do think the structure should be different

If it makes you feel any better, so do I. My preference would have been to have the processor client and a lower-level base type in the core library, just the checkpoint stores in external packages to manage the dependences. However, I lost that argument 4 years ago. 😄

Unfortunately, this comes down to a different vision from the Azure SDK architecture board on the hierarchy and extensibility in our .NET packages. Should we ever decide to release alternative checkpoint stores, each will come with an opinionated EventProcessorClient implementation bundled in.

from azure-sdk-for-net.

@jsquire Thanks!

I appreciate the honest feedback.

While I also disagree with the board's decision, it is what it is and i'm sure there were various factors to consider.

I know that ClientDiagnostics is just an internal implementation, however to replicate it I would need to copy your code and hope nothing will change over time.
Logic is, that while checkpointing might be internal, it's a feature that exist, not following it's schema will create different models in the trace data which is hell to manage.

So wether I checkpoint using Blob, SQL, mongo or in-memory, the core tracing should be the same while I can always hook in to the diagnostics and add or modify on the fly.
Since all azure libraries are OTel ready just that core OTel and the OTel of the libraries will be fine.

Thanks again!

from azure-sdk-for-net.