feat: add net_ldap instrumentation by scbjans · Pull Request #1587 · open-telemetry/opentelemetry-ruby-contrib

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-09T13:12:45Z

+            @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.

I agree.

My naive thought was that it would be handled in net-ldap itself, but upon closer inspection I see that this instrumentation can leak e.g. passwords in cleartext.
I'll need some time to figure out which specific attributes to include.

Fixed and added test in 3d2ff26

I think a good starting point would be to capture the name of the operation being performed ie open, read, parse_pdu.

Spans name is set to the event name, e.g. search.net_ldap (see here for a list of possible span names)

Sure, but if you look at https://opentelemetry.io/docs/specs/semconv/http/http-spans/ as well as others the span name is usually defined as a templated string which uses the attributes to build the name.

Thank you, I really appreciate your time and feedback!

I've updated the span name to be LDAP <operation> (modeled after the default Rack instrumentation span naming ). I also found that before I always set span_kind to client; updated this in f0338fc to correctly set it to internal for internal spans as well.

No worries at all. 🙂 I feel that we have gotten the signal definition into a great shape which is consistent with already defined signals.

My last suggestion based on my last comment would be to add the ldap operation as an attribute ldap.operation.type which would have the same value as used in the span name template.

Thanks, added the attribute in f39b2de

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

+            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.

Good points. I was also wondering about SemConv as I'm reading through this. I asked the #opentelemetry-semantic-conventions channel on CNCF Slack for advice.

Heard back from the thread. There aren't any specific LDAP conventions we should adhere to. Instead, they suggested we use attributes from other relevant categories like network and server.

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

+            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?

Sorry, I've seem to have missed this earlier. Right now we use the method as prescribed in ruby-net-ldap

kaylareopelle

Hi @scbjans, thanks for opening this PR.

I'm not very familiar with LDAP, so I'll take another pass once I learn a bit more. For now, I left a few comments.

In addition to those comments, would you mind updating the CODEOWNERS file to list you as the owner of this gem? PR assignment for CODEOWNERS is currently broken, but I'm hoping to get it fixed soon.

kaylareopelle · 2025-09-16T23:52:59Z

+gem 'opentelemetry-api'
+gem 'opentelemetry-common'
+gem 'opentelemetry-instrumentation-base'


should be able to leave these out, since they're dependencies of the other gems.

Suggested change

gem 'opentelemetry-api'

gem 'opentelemetry-common'

gem 'opentelemetry-instrumentation-base'

kaylareopelle · 2025-09-17T00:04:20Z

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


Good points. I was also wondering about SemConv as I'm reading through this. I asked the #opentelemetry-semantic-conventions channel on CNCF Slack for advice.

thompson-tomo · 2025-09-17T07:13:36Z

+
+          def instrument(event, payload)
+            attributes = {
+              'ldap.auth' => auth.except(:password).to_json,


Do we need auth given we removed it from db etc?

I'm not sure why it was removed from db, but I think it's valuable to see which user connects to ldap; e.g. to troubleshoot permissions

I believe it was in relation to the range of different auth methods/mechanism that could be used ie certificates vs user credentials.

The name Auth doesn't feel scoped enough as it could mean many things. In ldap v3 SASL is supported which opens up additional techniques.

I'm sorry for the slow turnaround.

In my testsetup, this currently produces: ldap.auth: { method: "simple", username: "<username-of-ldap-bind-user>" }.
This is directly based on the auth hash of the net-ldap gem
Would it be better to have ldap.auth.method and ldap.auth.username? Or shouldn't they be declared under ldap.auth at all?

If we retain it, then we should go with ldap.auth.method etc

Renamed in 97837bc

scbjans · 2025-09-17T09:57:41Z

@kaylareopelle thanks for your feedback; I updated it in 7264acb

@thompson-tomo thank you as well for your feedback, I believe I addressed it in 44a1d54
However, tests are failing, because the new opentelemetry-semantic_conventions is missing some requires of the SemConv module. I'll have to tidy up my fixes for that and submit a PR

thompson-tomo · 2025-09-18T03:03:15Z

+          def instrument(event, payload)
+            attributes = {
+              'ldap.auth' => auth.except(:password).to_json,
+              'ldap.base' => base,


Should this become ldap.tree.base or similar that way we have potential of adding other properties for the tree.

I don't foresee that yet, but it's probably best to be futureproof. I'll make that change.

Done so in dc069a9

thompson-tomo · 2025-09-18T03:08:52Z

+            attributes = {
+              'ldap.auth' => auth.except(:password).to_json,
+              'ldap.base' => base,
+              'ldap.encryption' => encryption.to_json,


Not sure of the benefits of this given it could be a hash of properties.

This is directly based on the encryption hash of the net-ldap gem
In my testsetup, this currently produces: ldap.encryption: { method: "simple_tls", tls_options: { verify_mode: 1 } }. I can imagine this should better fit under
the TLS namespace, however, I don't know how we should define attributes like verify_mode, or ca_file. Might be best to leave it out (at least for now)?

Agree best to leave it out for now and can be added if need arises but use the tls namespace where appropriate.

Removed it in 97837bc

thompson-tomo · 2025-10-08T10:49:51Z

+              'ldap.auth' => auth.except(:password).to_json,
+              'ldap.tree.base' => base,
+              'ldap.encryption' => encryption.to_json,
+              'ldap.request.message' => payload.to_json,
+              OpenTelemetry::SemConv::SERVER::SERVER_ADDRESS => host || hosts,
+              OpenTelemetry::SemConv::SERVER::SERVER_PORT => port,
+              OpenTelemetry::SemConv::Incubating::PEER::PEER_SERVICE => instrumentation_config[:peer_service],
+              OpenTelemetry::SemConv::NETWORK::NETWORK_PROTOCOL_NAME => 'ldap'
+            }


Looking at the semconv docs, is there potential for us to set network.transport as well as potentially the network.peer.* atttibutes?

net-ldap only supports tcp, so I've added that in 97837bc. Since the net-ldap gem abstracts away some connection details, I don't see an easy way to add the network.peer.* attributes

thompson-tomo · 2025-10-08T10:51:00Z

+              OpenTelemetry::SemConv::SERVER::SERVER_ADDRESS => host || hosts,
+              OpenTelemetry::SemConv::SERVER::SERVER_PORT => port,
+              OpenTelemetry::SemConv::Incubating::PEER::PEER_SERVICE => instrumentation_config[:peer_service],
+              OpenTelemetry::SemConv::NETWORK::NETWORK_PROTOCOL_NAME => 'ldap'


Is there a way to also include the protocol version?

Done in 97837bc

thompson-tomo

Span definition & attribute usage looks good especially as it is consistent with existing sem conv definitions. 👍

Will allow others to validate the coding patterns/structure.

scbjans · 2025-12-02T14:27:00Z

@arielvalentin @kaylareopelle I believe this PR is ready; could I please get your eyes on this again?

arielvalentin · 2026-01-06T01:26:39Z

+      module LDAP
+        # attribute mapper to redact sensitive keys
+        class AttributeMapper
+          SENSITIVE_KEYS = %w[userPassword unicodePwd lmPassword msDS-ManagedPassword ntPassword authPassword krbPrincipalKey sambaNTPassword sambaLMPassword].freeze


I think it is safer to have an allow list as opposed to a deny list.

This will prevent any new sensitive keys from sneaking in.

agreed, changed to an allowlist in bce45ff

@jdehaan @Hareramrai could you please have a look if there are any attributes you are missing in attribute_mapper.rb

scbjans · 2026-01-12T10:51:58Z

+
+  # ostruct is not part of the standard library from Ruby4+
+  # from net-ldap 0.20+ it is defined as a gem dependency
+  gem 'ostruct' if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new('4')


For net-ldap 0.20+ ostruct is defined as a gem dependency.
I'm also fine with updating the compatible block, so that only net-ldap 0.20+ is supported, if that is preferred

According to this comment the problem was with an old version of rake requiring openstruct. I'll update the Appraisals to keep the compatibility, and move openstruct there

arielvalentin

One last question regarding exception handling. After that I think the 6 month long journey comes to an end.

scbjans · 2026-01-13T07:29:48Z

One last question regarding exception handling. After that I think the 6 month long journey comes to an end.

Thank you (and @kaylareopelle and @thompson-tomo) so much for helping me get this in a better shape!

kaylareopelle · 2026-01-30T23:49:55Z

Hi @scbjans, thank you for your patience with us on this PR. I'd love to get it merged in and released. There's a merge conflict with the release-please-config. Could you take a look at it?

…ectsids

…s to be parsed.

…, so do not record_exception again.

scbjans · 2026-01-31T01:54:04Z

Hi @scbjans, thank you for your patience with us on this PR. I'd love to get it merged in and released. There's a merge conflict with the release-please-config. Could you take a look at it?

@kaylareopelle no worries, merge conflict is fixed. Failing test seem unrelated.

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

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 marked this pull request as ready for review August 18, 2025 12:12

scbjans requested a review from arielvalentin August 27, 2025 09:00

kaylareopelle reviewed Sep 17, 2025

View reviewed changes

thompson-tomo reviewed Sep 17, 2025

View reviewed changes

scbjans commented Sep 17, 2025

View reviewed changes

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

thompson-tomo reviewed Sep 18, 2025

View reviewed changes

thompson-tomo reviewed Oct 8, 2025

View reviewed changes

thompson-tomo approved these changes Oct 9, 2025

View reviewed changes

kaylareopelle approved these changes Oct 20, 2025

View reviewed changes

scbjans force-pushed the 669-add-net_ldap-instrumentation branch from 9e45a84 to 93deed5 Compare November 3, 2025 14:49

scbjans closed this Nov 24, 2025

scbjans force-pushed the 669-add-net_ldap-instrumentation branch from 45edbb3 to 44f34c0 Compare November 24, 2025 08:30

scbjans commented Dec 11, 2025

View reviewed changes

Comment thread instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb Outdated

arielvalentin reviewed Jan 6, 2026

View reviewed changes

arielvalentin reviewed Jan 11, 2026

View reviewed changes

scbjans commented Jan 12, 2026

View reviewed changes

arielvalentin reviewed Jan 13, 2026

View reviewed changes

Comment thread instrumentation/net_ldap/lib/opentelemetry/instrumentation/net/ldap/instrumentation_service.rb Outdated

Comment thread instrumentation/net_ldap/test/opentelemetry/instrumentation/net/ldap/instrumentation_test.rb

arielvalentin approved these changes Jan 13, 2026

View reviewed changes

Comment thread instrumentation/net_ldap/Gemfile

scbjans added 14 commits January 31, 2026 02:23

feat: add net_ldap instrumentation

1280449

chore: add net_ldap to component_owners

350aec5

chore: do not include internal spans by default

a08e2cf

chore: instrumentation should not break when searching for binary obj…

f9832b4

…ectsids

chore: appease the cop

5701286

chore: do not add attribute when JSON could not be generated.

32fdee7

chore: feedback

2a6efa0

chore: use Set to avoid linear search, only map JSON when String need…

11ca2e6

…s to be parsed.

chore: fix bad merge, remove explicit bundler dependency

69c6855

chore: add ostruct for net-ldap <0.20.0 and Ruby >=4

d203a47

chore: do not appraise old net-ldap versions for ruby4-

8587622

chore: move ostruct to Gemfile instead of Appraisals

ebec67b

chore: also add logger to Gemfile

3e19968

chore: move ostruct to Appraisals. in_span already records exceptions…

7be0bd2

…, so do not record_exception again.

scbjans force-pushed the 669-add-net_ldap-instrumentation branch from 220f557 to 7be0bd2 Compare January 31, 2026 01:26

scbjans added 2 commits February 5, 2026 20:52

Merge branch 'main' into 669-add-net_ldap-instrumentation

58b9f83

Merge branch 'main' into 669-add-net_ldap-instrumentation

9b0f364

arielvalentin merged commit cc2eca1 into open-telemetry:main Feb 18, 2026
45 checks passed

scbjans deleted the 669-add-net_ldap-instrumentation branch February 20, 2026 07:26

otelbot-ruby-contrib Bot mentioned this pull request Feb 22, 2026

release: Release opentelemetry-instrumentation-net_ldap 0.1.0 (initial release) #2010

Merged

	gem 'opentelemetry-api'
	gem 'opentelemetry-common'
	gem 'opentelemetry-instrumentation-base'

Conversation

scbjans commented Jul 9, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

kaylareopelle left a comment

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!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

scbjans commented Sep 17, 2025

Uh oh!

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 Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

scbjans Oct 8, 2025 •

edited

Loading