Apply service naming flow to messaging integrations #2961

jbertran · 2023-04-03T14:13:37Z

What does this PR do?

Apply the new naming flow introduced in #2941 to all messaging integrations.

In source, this just requires using the TracingPlugin methods for naming resolution.

In tests, this means defining the expected naming per version and performing relevant tests for both v0 and v1, using the withNamingSchema fixture.

Motivation

After the introduction of #2941 , we want all plugins to be sensitive to the naming schema.

Additional Notes

github-actions · 2023-04-03T14:14:49Z

Overall package size

Self size: 4.03 MB
Deduped: 63.28 MB
No deduping: 63.32 MB

Dependency sizes

name	version	self size	total size
@datadog/native-iast-taint-tracking	1.4.0	14.89 MB	14.9 MB
@datadog/pprof	2.2.0	13.71 MB	14.59 MB
@datadog/native-appsec	2.0.0	12.33 MB	12.34 MB
@datadog/native-metrics	1.6.0	7.88 MB	7.89 MB
protobufjs	7.1.2	2.76 MB	6.55 MB
@datadog/native-iast-rewriter	2.0.1	2.09 MB	2.1 MB
opentracing	0.14.7	194.81 kB	194.81 kB
semver	7.3.8	88.2 kB	118.6 kB
@datadog/sketches-js	2.1.0	109.9 kB	109.9 kB
lodash.sortby	4.7.0	75.76 kB	75.76 kB
lru-cache	7.14.0	74.95 kB	74.95 kB
ipaddr.js	2.0.1	59.52 kB	59.52 kB
ignore	5.2.0	48.87 kB	48.87 kB
import-in-the-middle	1.3.5	34.34 kB	38.81 kB
istanbul-lib-coverage	3.2.0	29.34 kB	29.34 kB
retry	0.10.1	27.44 kB	27.44 kB
lodash.uniq	4.5.0	25.01 kB	25.01 kB
limiter	1.1.5	23.17 kB	23.17 kB
lodash.kebabcase	4.1.1	17.75 kB	17.75 kB
lodash.pick	4.4.0	16.33 kB	16.33 kB
node-abort-controller	3.0.1	14.33 kB	14.33 kB
crypto-randomuuid	1.0.0	11.18 kB	11.18 kB
diagnostics_channel	1.1.0	7.07 kB	7.07 kB
path-to-regexp	0.1.7	6.78 kB	6.78 kB
koalas	1.0.2	6.47 kB	6.47 kB
methods	1.1.2	5.29 kB	5.29 kB
module-details-from-path	1.0.3	4.47 kB	4.47 kB

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}

codecov · 2023-04-03T14:33:06Z

Codecov Report

Merging #2961 (50b1fe7) into jbertran/service-naming-framework (e1db72b) will decrease coverage by 0.16%.
The diff coverage is 31.48%.

@@                          Coverage Diff                          @@
##           jbertran/service-naming-framework    #2961      +/-   ##
=====================================================================
- Coverage                              87.20%   87.04%   -0.16%     
=====================================================================
  Files                                    324      324              
  Lines                                  11583    11610      +27     
  Branches                                  33       33              
=====================================================================
+ Hits                                   10101    10106       +5     
- Misses                                  1482     1504      +22

Impacted Files	Coverage Δ
packages/dd-trace/src/plugins/client.js	`50.00% <0.00%> (-16.67%)`	⬇️
packages/dd-trace/src/plugins/outbound.js	`50.00% <ø> (+4.54%)`	⬆️
packages/dd-trace/src/plugins/tracing.js	`57.14% <0.00%> (ø)`
.../dd-trace/src/service-naming/schemas/definition.js	`30.00% <0.00%> (ø)`
packages/dd-trace/src/service-naming/schemas/v0.js	`13.04% <0.00%> (-29.82%)`	⬇️
packages/dd-trace/src/service-naming/schemas/v1.js	`35.71% <20.00%> (-14.29%)`	⬇️
packages/dd-trace/src/service-naming/index.js	`66.66% <33.33%> (+3.03%)`	⬆️
packages/datadog-plugin-amqp10/src/consumer.js	`88.88% <100.00%> (ø)`
packages/datadog-plugin-amqp10/src/producer.js	`90.90% <100.00%> (ø)`
packages/datadog-plugin-amqplib/src/client.js	`100.00% <100.00%> (ø)`
... and 9 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pr-commenter · 2023-04-03T14:41:48Z

Benchmarks

Comparing candidate commit 50b1fe7 in PR branch jbertran/service-naming-messaging with baseline commit e1db72b in branch jbertran/service-naming-framework.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 659 metrics, 49 unstable metrics.

tlhunter · 2023-04-18T22:22:19Z

packages/datadog-plugin-amqp10/src/consumer.js

-        'amqp.connection.user': address.user
-      }
-    })
+    this.startSpan(


I remember you and Roch talking about minimizing the line diffs in the plugins. Did the linter complain about this:

this.startSpan(this.operationName(), {

Oh, I see you did follow this pattern later in the PR. Looks like there are several places where this can be applied.

This PR was a follow-up to #2941 (which is the one we iterated on). I was waiting on the merge of #2941 to rebase this one on top, to minimize the amount of extra work, but it sounds like we're pretty close so I'll take care of it now.

tlhunter · 2023-04-18T22:39:35Z

packages/dd-trace/src/plugins/tracing.js

Do you know if we're lucky enough now that all of the plugins pass this.operationName() as the first argument and the same .service pattern in the second argument?

If we were lucky then we could refactor startSpan like so:

startSpan ({ childOf, kind, meta, metrics, resource, type } = {}) { name = this.operationName() service = this.config.service || this.serviceName({ service: this.tracer._service }) const store = storage.getStore()

If not, I then wonder if it's a common enough pattern that would could introduce some sort of wrapper that maybe filled in the blanks:

startSpanWithDefaults (obj) { let name = this.operationName() let service = this.config.service || this.serviceName({ service: this.tracer._service }) return this.startSpan(name, {...obj, service}) }

Then again, it might just be another annoying layer of abstraction. Wdyt @rochdev?

Name and service are indeed common as far as I've seen, it's just that the service name override config.service || this.serviceName occurs at different levels in the hierarchy depending on the plugins. I think we should be able to make this behaviour common, but I'd like to update all relevant plugins to use the naming schema before tackling that, to have a clearer picture of where the pitfalls are (if any).

tlhunter · 2023-04-18T22:44:15Z

packages/dd-trace/test/plugin_manager.spec.js

@@ -240,6 +240,17 @@ describe('Plugin Manager', () => {
      loadChannel.publish({ name: 'four' })
      expect(instantiated).to.deep.equal(['two', 'four'])
    })
+    it('configures service naming schema resolution', () => {


I wonder if we should break this up:

const Nomenclature = require('../../dd-trace/src/service-naming') // top of file? describe('configures service naming schema resolution', () => { const config = { foo: { 'bar': 1 }, baz: 2 } let configureSpy beforeEach(() => { configureSpy = sinon.spy(Nomenclature, 'configure') }) afterEach(() => { configureSpy.restore() }) it('works', () => { pm.configure(config) expect(configureSpy).to.have.been.calledWith(config) }) })

The expect() call throws IIRC, and basically the test can fail for other reasons and not get to the part with the restore. By throwing the cleanup in an afterEach (or after) it then prevents a situation where a failed expectation pollutes the future state of the tests.

Done in #2941

tlhunter · 2023-04-18T22:46:32Z

I left a bunch of small comments but I think it's architecturally good and overall pretty close.

rochdev

The overall design LGTM, just a few comments on minor things that could be improved, especially given that this will touch every plugin eventually.

packages/datadog-plugin-amqp10/src/consumer.js

rochdev · 2023-04-19T20:45:17Z

packages/datadog-plugin-amqp10/test/index.spec.js

+
+          withNamingSchema(
+            () => sender.send({ key: 'value' }),
+            () => namingSchema.send.opName,


Do these need to be functions? Accepting an object could make this easier also when an override is not needed, for example withNamingSchema(() => {}, namingSchema.send).

With the way the tests are performed, they do need to be functions. I'm open to other ideas since I'm not a huge fan either 😅

namingSchema is a proxy that resolves the v0/v1 part of the naming schema, so it must be accessed inside the test case, because the configuration change that controls v0/v1 is kept at the test case level.

rochdev · 2023-04-19T20:54:45Z

packages/datadog-plugin-amqplib/src/client.js

@@ -7,15 +7,17 @@ const { getResourceName } = require('./util')

 class AmqplibClientPlugin extends ClientPlugin {
  static get id () { return 'amqplib' }
+  static get type () { return 'messaging' }
+  static get ioDirection () { return 'controlPlane' }


Not sure I understand this, that definitely doesn't sound like a direction?

Also, is this actually used somewhere?

Agreed, this ioDirection should probably be more generically named. subType sounds a bit too generic to me, WDYT?

Of all 3 AMQP integrations, this one is special because it's auto-instrumented based on a defs.js file generated by the library, which includes control-plane calls for managing exchanges and queues. These get their own amqp.command operation name, which has to go somewhere that's neither inbound nor outbound in the naming schema.

The fact that it's a control plane then can be inferred by type=worker + kind=client.

Done in 50b1fe7. Client integrations will get updated, so I'll generalize this part to client integrations when they're all updated.

rochdev · 2023-04-19T20:58:26Z

packages/datadog-plugin-amqplib/src/client.js

@@ -7,15 +7,17 @@ const { getResourceName } = require('./util')

 class AmqplibClientPlugin extends ClientPlugin {
  static get id () { return 'amqplib' }
+  static get type () { return 'messaging' }


This is redundant with both the span kind and the IO direction, and could be set in the base class for most cases. Can we settle on just 1 and make sure that everything is normalized accordingly?

Span kind can be integrated in startSpan for consumer and producer plugins. For this specific one, since it's a client we can't bake it in until we've dealt with all clients, and we're in the rare case where the client is specifically a messaging client (most will be HTTP or database), so setting it here seems like the best we can do.

Done in 05ad286

* add v0 to all messaging plugins * add v1 to all messaging plugins * test naming schema for all messaging integrations * add naming schema tests for other versions * bake messaging data into producer/consumer plugins * persist kind in plugins and infer naming subtype from kind+type

This reverts commit 4c0e856.

* add v0 to all messaging plugins * add v1 to all messaging plugins * test naming schema for all messaging integrations * add naming schema tests for other versions * bake messaging data into producer/consumer plugins * persist kind in plugins and infer naming subtype from kind+type

* introduce DD_TRACE_SPAN_ATTRIBUTE_SCHEMA env var * add attribute schema v0 for rhea * add attribute schema v1 for rhea * add test harness for service/operation naming * grab config object from pluginManager init * provide schema autoresolution in consumer/producer plugins * bind service to schema manager at configure time * rename plugins to match inbound/outbound service naming terminology * minimize test dependencies * naming resolution wrt version in the test object uses the Nomenclature, instead of being resolved by the test fixture * we no longer use the test fixture for _all_ existing tests, rather let the default resolution do the work * the testing fixture accepts a callback which is a minimal viable trace retrieval, on which we examine _only_ service and name * split test naming schema from test code * Apply service naming flow to messaging integrations (#2961) * add v0 to all messaging plugins * add v1 to all messaging plugins * test naming schema for all messaging integrations * add naming schema tests for other versions * bake messaging data into producer/consumer plugins * persist kind in plugins and infer naming subtype from kind+type * no logs on empty DD_SPAN_TRACE_ATTRIBUTE_SCHEMA * don't compute service name unless necessary

jbertran requested a review from a team as a code owner April 3, 2023 14:13

jbertran added the semver-minor label Apr 3, 2023

jbertran force-pushed the jbertran/service-naming-messaging branch from d5ab814 to d0a8c94 Compare April 3, 2023 14:27

jbertran force-pushed the jbertran/service-naming-framework branch from bc2afe9 to 9d58aba Compare April 4, 2023 12:33

jbertran force-pushed the jbertran/service-naming-messaging branch from d0a8c94 to 58f0dca Compare April 4, 2023 13:02

jbertran force-pushed the jbertran/service-naming-framework branch from 9d58aba to 666ed7a Compare April 4, 2023 14:16

jbertran force-pushed the jbertran/service-naming-messaging branch from 58f0dca to 4d3c8be Compare April 4, 2023 15:32

jbertran force-pushed the jbertran/service-naming-framework branch from 666ed7a to d6c89eb Compare April 5, 2023 09:10

jbertran force-pushed the jbertran/service-naming-messaging branch from 4d3c8be to 3f71489 Compare April 5, 2023 09:13

jbertran force-pushed the jbertran/service-naming-framework branch 2 times, most recently from 53d730b to 0b51015 Compare April 12, 2023 09:40

jbertran force-pushed the jbertran/service-naming-framework branch from 8122b35 to 59c42f3 Compare April 17, 2023 09:32

tlhunter self-assigned this Apr 17, 2023

jbertran force-pushed the jbertran/service-naming-framework branch from 2a75c6c to ae92f15 Compare April 18, 2023 15:13

tlhunter reviewed Apr 18, 2023

View reviewed changes

jbertran force-pushed the jbertran/service-naming-messaging branch 4 times, most recently from 447629c to 2e1360a Compare April 19, 2023 14:37

jbertran requested a review from tlhunter April 19, 2023 14:43

tlhunter approved these changes Apr 19, 2023

View reviewed changes

rochdev requested changes Apr 19, 2023

View reviewed changes

jbertran force-pushed the jbertran/service-naming-framework branch 2 times, most recently from 156b625 to ca5d3e1 Compare April 20, 2023 14:34

jbertran force-pushed the jbertran/service-naming-framework branch from ca5d3e1 to e1db72b Compare April 25, 2023 12:14

Jordi Bertran de Balanda added 5 commits April 25, 2023 14:15

add v0 to all messaging plugins

765bb2d

add v1 to all messaging plugins

4a9ca82

test naming schema for all messaging integrations

23fe077

add naming schema tests for other versions

123ea5c

bake messaging data into producer/consumer plugins

78b3994

jbertran force-pushed the jbertran/service-naming-messaging branch 2 times, most recently from a27b9d1 to 427f07e Compare April 25, 2023 13:20

persist kind in plugins and infer naming subtype from kind+type

50b1fe7

jbertran force-pushed the jbertran/service-naming-messaging branch from 427f07e to 50b1fe7 Compare April 25, 2023 15:05

jbertran merged commit 13521f0 into jbertran/service-naming-framework Apr 26, 2023

jbertran mentioned this pull request Apr 27, 2023

Cache integrations - service naming #3056

Merged

1 task

jbertran pushed a commit that referenced this pull request May 4, 2023

Revert "Apply service naming flow to messaging integrations (#2961)"

3d82e16

This reverts commit 4c0e856.

jbertran mentioned this pull request May 16, 2023

Service Naming API #3161

Merged

This was referenced Jun 1, 2023

v2.35.0 proposal #3206

Merged

v4.1.0 proposal #3209

Merged

v3.22.0 proposal #3207

Merged

tlhunter deleted the jbertran/service-naming-messaging branch January 19, 2024 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply service naming flow to messaging integrations #2961

Apply service naming flow to messaging integrations #2961

jbertran commented Apr 3, 2023 •

edited

Loading

github-actions bot commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

pr-commenter bot commented Apr 3, 2023 •

edited

Loading

tlhunter Apr 18, 2023

tlhunter Apr 18, 2023

jbertran Apr 19, 2023

tlhunter Apr 18, 2023

jbertran Apr 19, 2023

tlhunter Apr 18, 2023

jbertran Apr 19, 2023

tlhunter commented Apr 18, 2023

rochdev left a comment

rochdev Apr 19, 2023

jbertran Apr 20, 2023

rochdev Apr 19, 2023

rochdev Apr 19, 2023

jbertran Apr 20, 2023

rochdev Apr 24, 2023

jbertran Apr 25, 2023 •

edited

Loading

rochdev Apr 19, 2023

jbertran Apr 20, 2023

jbertran Apr 20, 2023

Apply service naming flow to messaging integrations #2961

Apply service naming flow to messaging integrations #2961

Conversation

jbertran commented Apr 3, 2023 • edited Loading

What does this PR do?

Motivation

Additional Notes

github-actions bot commented Apr 3, 2023 • edited Loading

Overall package size

Dependency sizes

codecov bot commented Apr 3, 2023 • edited Loading

Codecov Report

pr-commenter bot commented Apr 3, 2023 • edited Loading

Benchmarks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlhunter commented Apr 18, 2023

rochdev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbertran Apr 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbertran commented Apr 3, 2023 •

edited

Loading

github-actions bot commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 3, 2023 •

edited

Loading

pr-commenter bot commented Apr 3, 2023 •

edited

Loading

jbertran Apr 25, 2023 •

edited

Loading