feat: add net_ldap instrumentation #1587

scbjans · 2025-07-09T12:08:10Z

Largely based on prior work in #1432 by @Hareramrai and @jdehaan

Since that PR seems stale, I've updated it with some small edits to comply with release management using toys.

Should fix: github.com//issues/669

arielvalentin · 2025-07-09T12:40:56Z

.toys/.data/releases.yml

@@ -231,7 +237,8 @@ gems:

  - name: opentelemetry-propagator-google_cloud_trace_context
    directory: propagator/google_cloud_trace_context
-    version_constant: [OpenTelemetry, Propagator, GoogleCloudTraceContext, VERSION]
+    version_constant:


Why did you add new lines to the GCP related definitions?

Thanks for your quick review @arielvalentin !

I guess this is from the format on save option in VSCode which I accidentally committed. I'll revert it.

I'll respond to the other comments this Friday.

arielvalentin · 2025-07-09T12:42:36Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation.rb

+      module LDAP
+        # The Instrumentation class contains logic to detect and install the Net::LDAP
+        # instrumentation
+        class Instrumentation < OpenTelemetry::Instrumentation::Base


Is there a min version requirement?

FYI: We tested the original implementation using gem 'net-ldap' 0.17.0 if that info helps in finding out some requirements. As it's a 0.x version we assumed all bets are off in this case....

Thanks, I only tested this with net-ldap 0.19.0. I'm inclined to say we should set a minimal version to 0.17.1, since in that version Ruby3.x support was added (as in added to CI), since our minimum Ruby version is 3.1 (which is also EOL, shall I put up a PR for that as well?)
cb5f1c4

That sounds very reasonable to me, we are also planning to upgrade anyways soon.

arielvalentin · 2025-07-09T12:43:30Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+              yield(payload).tap do |response|
+                annotate_span_with_response!(span, response)
+              end
+            rescue StandardError => e


Doesn't in_span already handle Error Recording and setting the status?

Yes it does, but it doesn't add the ldap.error_message attribute. I've updated this a bit, and in 0583992 we'll rescue specifically on a ::Net::LDAP::Error

arielvalentin · 2025-07-09T12:44:39Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+            LDAP::Instrumentation.instance.config
+          end
+
+          def annotate_span_with_response!(span, response)


Only add a ! if you have an existing method that does not have a !

arielvalentin · 2025-07-09T12:47:20Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+
+            return if ::Net::LDAP::ResultCodesNonError.include?(status_code)
+
+            span.record_exception(error_message) unless error_message.empty?


I believe record_exception is an explicit helper for exception events. Consider using the Span Status Code description instead.

arielvalentin · 2025-07-09T12:58:19Z

release-please-config.json

@@ -227,4 +231,4 @@
      "version-file": "lib/opentelemetry/resource/detector/google_cloud_platform/version.rb"
    }
  }
-}
+}


Suggested change

}

}

arielvalentin · 2025-07-09T13:10:03Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+              OpenTelemetry::SemanticConventions::Trace::NET_PEER_NAME => host || hosts,
+              OpenTelemetry::SemanticConventions::Trace::NET_PEER_PORT => port,
+              OpenTelemetry::SemanticConventions::Trace::PEER_SERVICE => instrumentation_config[:peer_service]
+            }.merge!(OpenTelemetry::Common::HTTP::ClientContext.attributes)


why is this amending HTTP Client attributes? How is that relevant to LDAP lookups?

TBH I just took the already existing stale PR that originally was opened to add this instrumentation. That did the job for me, and I thought it might be beneficial for others to have this instrumentation more easily available.
I rushed this a bit and skipped the due diligence (sorry!), so I'm glad you ask.

It doesn't: I removed it.

arielvalentin · 2025-07-09T13:12:45Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+            @encryption = args[:encryption]
+          end
+
+          def instrument(event, payload)


Do we want more specific attribute mappers here for each of these events?

https://github.com/search?q=repo%3Aruby-ldap%2Fruby-net-ldap+%2Finstrument+%22%2F&type=code

I do not particularly like the practice of dumping json strings where we do not know if the contents of the provided payload have sensitive information in them.

arielvalentin · 2025-07-09T13:16:59Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+
+            tracer.in_span(
+              event,
+              attributes: attributes.compact,


This allocates and extra hash object.

lets avoid that by checking if the values are present before adding them to the hash.

arielvalentin · 2025-07-09T13:18:50Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb

+            tracer.in_span(
+              event,
+              attributes: attributes.compact,
+              kind: :client


Do you think LDAP qualifies as RPC? Would using RPC client semantics be right for this?
Would using ECS style events make more sense for these?

I was inclined to say LDAP qualifies as RPC, but my AI friend does not agree with me. Apparently LDAP itself is not RPC, but it is often combined with RPC (e.g. when joining computers to Windows domains), but we're only instrumenting LDAP in this case. Semconv doesn't specify anything for ldap, and other languages don't seem to have ldap instrumentation.

I'm not sure what to do here.

arielvalentin · 2025-07-09T16:17:50Z

instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/patches/instrumentation.rb

+            def initialize(args = {})
+              super
+
+              @instrumentation_service = args[:instrumentation_service] || OpenTelemetry::Instrumentation::Net::LDAP::InstrumentationService.new({


Another thought I have here, what if we added this otel instrumentation as a decorator to the injected service?

feat: add net_ldap instrumentation

623e8f0

scbjans requested review from fbogsany, mwear, robertlaurin, dazuma, ericmustin, arielvalentin, ahayworth, plantfansam, robbkidd, simi, kaylareopelle, xuan-cao-swi and a team as code owners July 9, 2025 12:08

scbjans added 2 commits July 9, 2025 14:10

chore: instrumentation-net_ldap should not yet be included in all

addcd6d

chore: add missing appraisals

e147bbe

arielvalentin reviewed Jul 9, 2025

View reviewed changes

scbjans marked this pull request as draft July 9, 2025 14:30

arielvalentin reviewed Jul 9, 2025

View reviewed changes

scbjans added 2 commits July 11, 2025 10:11

chore: feedback

0583992

chore: add version constraint >=0.17.1 for net-ldap

cb5f1c4


		return if ::Net::LDAP::ResultCodesNonError.include?(status_code)

		span.record_exception(error_message) unless error_message.empty?

feat: add net_ldap instrumentation #1587

Are you sure you want to change the base?

feat: add net_ldap instrumentation #1587

Conversation

scbjans commented Jul 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scbjans Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scbjans Jul 11, 2025 •

edited

Loading