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

🚀 Feature: make "ImageUploader" pluggable. #2324

Open
1 task done
michaelsafyan opened this issue Nov 22, 2024 · 2 comments
Open
1 task done

🚀 Feature: make "ImageUploader" pluggable. #2324

michaelsafyan opened this issue Nov 22, 2024 · 2 comments
Labels
enhancement New feature or request

Comments

@michaelsafyan
Copy link

Which component is this feature for?

Traceloop SDK

🔖 Feature description

More clearly define the interface of ImageUploader, make it an abstract base class (moving current implementation to a TraceloopImageUploader), and define a mechanism for constructing/selecting/plugging different implementations (as well as guidance regarding where different implementations of the base class ought to live).

🎤 Why is this feature needed ?

In order to allow this feature to work with other backends/vendors, there needs to be a way to swap out different implementations.

For some additional context, I'm currently working on sample code for Google Cloud / OpenLLMetry integration:

As part of that, I am implementing an alternative ImageUploader:

There is a bit of a code smell, though, in that it would seem wrong to inherit from traceloop.sdk.ImageUploader due to traceloop.sdk.ImageUploader not being purely abstract. On the other hand, simply following the shape/outline of traceloop.sdk.ImageUploader without inheritance seems error prone.

Separately from the above concern, I am thinking about where such an implementation ought to live in the long run and how to make alternative implementations of this easily discoverable. Where images get stored could conceivably be orthogonal to which observability backend is used; for example, as long as there was a way to configure the ACLs to grant the observability backend the necessary access, the observability backend and storage backend might be different [e.g. a Traceloop user storing images in GCS; a Google Cloud Observability user storing images in S3].

✌️ How do you aim to achieve this?

It would be a good start to just split ImageUploader into an abstract base class and a concrete, default implementation.

Beyond this, clearer comments on the interface to make the meaning clear. Perhaps some slight renaming (e.g. from image_file to base64_data_string, if I understand its format correctly). Type annotations, too.

Then, after that, perhaps some structure around how to organize implementations of it. Perhaps putting the concrete implementation in a separate file in a subfolder for implementations, along with instructions on where/how to contribute alternative implementations.

🔄️ Additional Information

No response

👀 Have you spent some time to check if this feature request has been raised before?

  • I checked and didn't find similar issue

Are you willing to submit PR?

Yes I am willing to submit a PR!

@dosubot dosubot bot added the enhancement New feature or request label Nov 22, 2024
@nirga
Copy link
Member

nirga commented Nov 25, 2024

Sounds good @michaelsafyan! You marked you're willing to make this PR so happy to support you in the process :)
I agree this should be abstract - we're coming from golang where abstract classes don't really exist so that's why we chose this "duck-typing" design :)

@michaelsafyan
Copy link
Author

Thanks, Nir!

I've proposed the shape of a solution here:

The more I think about it, the more I'm inclined to approach this by:

  1. Establishing the interfaces first in OTel Contrib Python.
  2. Migrating the existing ImageUploader implementation in OpenLLMetry to that interface.

Thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants