search

open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.96k stars 858 forks source link

Support folsom's Memcached client #9028

Open evg-tso opened 1 year ago

evg-tso commented 1 year ago

Is your feature request related to a problem? Please describe. The spotify/folsom Memcached client supports custom metrics and tracing through interfaces.
I think adding support for OpenTelemetry for folsom can be rather easily created.

Describe the solution you'd like Folsom has built-in support for micrometer and opencensus, so adapting them to OpenTelemetry's spec should be rather simple.

Describe alternatives you've considered There are several options:

  1. Use OTEL's micrometer adapter and use folsom's built-in micrometer module, but they don't report the MemcacheStatus as a tag, which is a great enum to have in the counters/timers.
  2. Implement the OTEL's implementation in folsom's repo as a separate module. See relevant issue: https://github.com/spotify/folsom/issues/222.
  3. Implement it here, which can later be expanded into a java-agent.
trask commented 1 year ago

just wanted to mention that going with #2 doesn't preclude reusing it in java agent instrumentation, see azure-core and couchbase-3.2 instrumenations for example

evg-tso commented 1 year ago

If you are in favor of this addition, I'd like to tackle it and work on a PR.
I'd appreciate your thoughts on the proper code location, either here or in folsom's repo.

mateuszrzeszutek commented 1 year ago

I'd appreciate your thoughts on the proper code location, either here or in folsom's repo.

Looks like the folsom repo already contains several implementations for different metric libraries. WDYT about adding instrumentation there? You can tag me and Trask there, we'll help you review the OTel API usage patterns.

And after that gets merged, we can add a javaagent module that'll automatically apply the instrumentation.

evg-tso commented 1 year ago

Cool, I'll work on it

korzepadawid commented 1 year ago

It seems abandoned, but I've implemented the support for OTel metrics in spotify/folsom, the PR https://github.com/spotify/folsom/pull/229. Feedback appreciated.

@trask @mateuszrzeszutek

mateuszrzeszutek commented 1 year ago

Thanks @korzepadawid ! I left a couple of comments in that PR.

korzepadawid commented 1 year ago

Thank you @mateuszrzeszutek for the review. The PR with metrics is merged https://github.com/spotify/folsom/pull/229.

korzepadawid commented 1 year ago

I've been thinking about tracing implementation, I might work on that in the future.

mateuszrzeszutek commented 1 year ago

I've been thinking about tracing implementation, I might work on that in the future.

👍 Let us know if you open a new PR for that, we'll keep this issue open in the meantime.