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

Introduce service naming framework #2941

Merged
merged 11 commits into from
Apr 27, 2023
Merged

Conversation

jbertran
Copy link
Contributor

@jbertran jbertran commented Mar 27, 2023

What does this PR do?

This PR:

  • Adds interaction with the DD_TRACE_SPAN_ATTRIBUTE_SCHEMA environment variable
  • Provides the framework for 2 versions of the service and operation naming schema:
    • v0 is the current version, i.e. a representation of the existing state of the plugin. One could checkout the commit with the introduction of this version, and have unchanged tests still pass.
    • v1 is the new naming schema, made consistent across all tracers.
  • Introduces a testing harness for easy tests of v0 and v1 naming
  • Applies this versioned naming for all messaging integrations - rhea, amqp(10|lib), kafkajs, google-cloud-pubsub

Motivation

This is part of the service representation initiative, which provides in an opt-in way a new manner of naming services and operations, and will introduce peer.service soon.

The opt-in nature of the change guarantee that we do not break naming for existing deployments.

@github-actions
Copy link

github-actions bot commented Mar 27, 2023

Overall package size

Self size: 4 MB
Deduped: 64.22 MB
No deduping: 64.26 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 3.1.0 13.31 MB 13.32 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
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Merging #2941 (4c0e856) into master (a0c2514) will decrease coverage by 0.30%.
The diff coverage is 52.12%.

@@            Coverage Diff             @@
##           master    #2941      +/-   ##
==========================================
- Coverage   87.19%   86.89%   -0.30%     
==========================================
  Files         317      304      -13     
  Lines       11512    10791     -721     
  Branches       33       33              
==========================================
- Hits        10038     9377     -661     
+ Misses       1474     1414      -60     
Impacted Files Coverage Δ
packages/dd-trace/src/service-naming/schemas/v0.js 13.04% <13.04%> (ø)
packages/dd-trace/src/plugins/tracing.js 57.14% <20.00%> (-6.20%) ⬇️
.../dd-trace/src/service-naming/schemas/definition.js 30.00% <30.00%> (ø)
packages/dd-trace/src/service-naming/schemas/v1.js 35.71% <35.71%> (ø)
packages/dd-trace/src/plugins/client.js 50.00% <50.00%> (-16.67%) ⬇️
packages/dd-trace/src/service-naming/index.js 66.66% <66.66%> (ø)
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%> (ø)
packages/datadog-plugin-amqplib/src/consumer.js 100.00% <100.00%> (ø)
... and 12 more

... and 18 files with indirect coverage changes

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

@jbertran jbertran marked this pull request as ready for review March 29, 2023 09:28
@jbertran jbertran requested a review from a team as a code owner March 29, 2023 09:28
@jbertran jbertran force-pushed the jbertran/service-naming-framework branch from aa422c8 to 1ce5b80 Compare March 30, 2023 13:24
packages/datadog-plugin-rhea/src/consumer.js Outdated Show resolved Hide resolved
packages/dd-trace/src/service-naming/schema.js Outdated Show resolved Hide resolved
packages/dd-trace/src/service-naming/schema.js Outdated Show resolved Hide resolved
packages/dd-trace/src/service-naming/schemas/v0.js Outdated Show resolved Hide resolved
packages/dd-trace/src/service-naming/schemas/v1.js Outdated Show resolved Hide resolved
@jbertran jbertran force-pushed the jbertran/service-naming-framework branch from 1ce5b80 to 0e7dd20 Compare March 31, 2023 13:48
@jbertran jbertran requested a review from tlhunter April 3, 2023 13:37
@jbertran jbertran force-pushed the jbertran/service-naming-framework branch 2 times, most recently from 9d58aba to 666ed7a Compare April 4, 2023 14:16
@jbertran jbertran requested a review from rochdev April 5, 2023 08:22
@jbertran jbertran force-pushed the jbertran/service-naming-framework branch 2 times, most recently from d6c89eb to e342157 Compare April 7, 2023 16:33
@jbertran
Copy link
Contributor Author

jbertran commented Apr 7, 2023

Ongoing to-do list:

  • Minimize diff in plugin source
  • Rename incoming/outgoing to match naming terminology (Inbound/Outbound)
  • Verify if plugins all currently fit into the plugin class inheritance tree (and if so, use instance class instead of instance methods to resolve naming)
    • No, even if some of them are trivial to make fit into the model (see HTTP for example)
  • Investigate ways to minimize test diff (too much indentation because we wrap the existing tests)
  • Pass service as a positional argument in service name resolution
    • Easy, but we don't go further than this (no trickery to freeze service name in the schema and prevent us from passing it altogether)
  • Investigate if moving to a literal is possible instead of a function in operation name resolution
    • Not in the current state of at least the couchbase plugin

@jbertran jbertran force-pushed the jbertran/service-naming-framework branch 4 times, most recently from 8122b35 to 59c42f3 Compare April 17, 2023 09:32
@DataDog DataDog deleted a comment from pr-commenter bot Apr 17, 2023
@pr-commenter
Copy link

pr-commenter bot commented Apr 17, 2023

Benchmarks

Comparing candidate commit 4c0e856 in PR branch jbertran/service-naming-framework with baseline commit a0c2514 in branch master.

Found 0 performance improvements and 28 performance regressions! Performance is the same for 639 metrics, 41 unstable metrics.

scenario:log-with-debug-14

  • 🟥 cpu_user_time [+7.634ms; +10.893ms] or [+5.656%; +8.071%]
  • 🟥 execution_time [+10.168ms; +11.128ms] or [+6.495%; +7.108%]

scenario:log-with-error-14

  • 🟥 cpu_user_time [+8.511ms; +11.487ms] or [+6.298%; +8.499%]
  • 🟥 execution_time [+9.899ms; +10.868ms] or [+6.314%; +6.932%]

scenario:log-skip-log-14

  • 🟥 cpu_user_time [+8.104ms; +11.315ms] or [+5.979%; +8.348%]
  • 🟥 execution_time [+10.726ms; +11.641ms] or [+6.846%; +7.430%]

scenario:log-without-log-14

  • 🟥 cpu_user_time [+8.886ms; +12.292ms] or [+6.581%; +9.102%]
  • 🟥 execution_time [+9.980ms; +10.976ms] or [+6.343%; +6.976%]

scenario:startup-with-tracer-14

  • 🟥 execution_time [+10.279ms; +11.172ms] or [+6.630%; +7.206%]

scenario:log-without-log-16

  • 🟥 cpu_user_time [+8.669ms; +11.831ms] or [+6.316%; +8.620%]
  • 🟥 execution_time [+10.562ms; +11.058ms] or [+6.573%; +6.881%]

scenario:log-with-debug-16

  • 🟥 cpu_user_time [+7.644ms; +10.731ms] or [+5.574%; +7.825%]
  • 🟥 execution_time [+10.015ms; +11.270ms] or [+6.243%; +7.025%]

scenario:log-with-error-16

  • 🟥 cpu_user_time [+7.783ms; +10.999ms] or [+5.673%; +8.017%]
  • 🟥 execution_time [+9.662ms; +10.777ms] or [+6.008%; +6.701%]

scenario:log-skip-log-16

  • 🟥 cpu_user_time [+7.160ms; +10.450ms] or [+5.218%; +7.615%]
  • 🟥 execution_time [+9.863ms; +11.190ms] or [+6.126%; +6.951%]

scenario:startup-with-tracer-16

  • 🟥 cpu_user_time [+9.955ms; +17.076ms] or [+7.406%; +12.703%]
  • 🟥 execution_time [+10.071ms; +10.750ms] or [+6.354%; +6.783%]

scenario:log-with-debug-18

  • 🟥 cpu_user_time [+8.164ms; +11.634ms] or [+5.866%; +8.359%]
  • 🟥 execution_time [+10.148ms; +11.490ms] or [+6.140%; +6.952%]

scenario:log-skip-log-18

  • 🟥 cpu_user_time [+7.454ms; +10.923ms] or [+5.343%; +7.830%]
  • 🟥 execution_time [+10.605ms; +11.385ms] or [+6.410%; +6.882%]

scenario:log-with-error-18

  • 🟥 cpu_user_time [+8.193ms; +11.511ms] or [+5.904%; +8.296%]
  • 🟥 execution_time [+10.017ms; +11.027ms] or [+6.053%; +6.663%]

scenario:log-without-log-18

  • 🟥 cpu_user_time [+8.202ms; +11.791ms] or [+5.902%; +8.485%]
  • 🟥 execution_time [+9.985ms; +11.293ms] or [+6.004%; +6.790%]

scenario:startup-with-tracer-18

  • 🟥 execution_time [+11.659ms; +12.246ms] or [+7.167%; +7.528%]

@jbertran jbertran force-pushed the jbertran/service-naming-framework branch 4 times, most recently from ca5d3e1 to e1db72b Compare April 25, 2023 12:14
@jbertran jbertran merged commit 1de7a30 into master Apr 27, 2023
109 checks passed
@jbertran
Copy link
Contributor Author

LGTM, but the benchmarks seem to disagree. Let's make sure that there is no regression but then it's fine to merge 👍

The benchmark workflows were resource-starved, rerunning was enough.

tlhunter added a commit that referenced this pull request Apr 27, 2023
CarlesDD pushed a commit that referenced this pull request Apr 28, 2023
* Add interaction with the `DD_TRACE_SPAN_ATTRIBUTE_SCHEMA` environment variable
* Provide the building blocks for 2 versions of an service and operation naming schema:
    * `v0` is the current version, i.e. a representation of the existing state of the plugin. One could checkout the commit with the introduction of this version, and have unchanged tests still pass.
    * `v1` is the new naming schema, made consistent across all tracers.
* Introduce a testing harness for easy tests of `v0` _and_ `v1` naming
* Apply this versioned naming for all messaging integrations - `rhea`, `amqp(10|lib)`, `kafkajs`, `google-cloud-pubsub`
CarlesDD pushed a commit that referenced this pull request Apr 28, 2023
* Add interaction with the `DD_TRACE_SPAN_ATTRIBUTE_SCHEMA` environment variable
* Provide the building blocks for 2 versions of an service and operation naming schema:
    * `v0` is the current version, i.e. a representation of the existing state of the plugin. One could checkout the commit with the introduction of this version, and have unchanged tests still pass.
    * `v1` is the new naming schema, made consistent across all tracers.
* Introduce a testing harness for easy tests of `v0` _and_ `v1` naming
* Apply this versioned naming for all messaging integrations - `rhea`, `amqp(10|lib)`, `kafkajs`, `google-cloud-pubsub`
CarlesDD pushed a commit that referenced this pull request Apr 28, 2023
* Add interaction with the `DD_TRACE_SPAN_ATTRIBUTE_SCHEMA` environment variable
* Provide the building blocks for 2 versions of an service and operation naming schema:
    * `v0` is the current version, i.e. a representation of the existing state of the plugin. One could checkout the commit with the introduction of this version, and have unchanged tests still pass.
    * `v1` is the new naming schema, made consistent across all tracers.
* Introduce a testing harness for easy tests of `v0` _and_ `v1` naming
* Apply this versioned naming for all messaging integrations - `rhea`, `amqp(10|lib)`, `kafkajs`, `google-cloud-pubsub`
CarlesDD pushed a commit that referenced this pull request Apr 28, 2023
* Add interaction with the `DD_TRACE_SPAN_ATTRIBUTE_SCHEMA` environment variable
* Provide the building blocks for 2 versions of an service and operation naming schema:
    * `v0` is the current version, i.e. a representation of the existing state of the plugin. One could checkout the commit with the introduction of this version, and have unchanged tests still pass.
    * `v1` is the new naming schema, made consistent across all tracers.
* Introduce a testing harness for easy tests of `v0` _and_ `v1` naming
* Apply this versioned naming for all messaging integrations - `rhea`, `amqp(10|lib)`, `kafkajs`, `google-cloud-pubsub`
jbertran added a commit that referenced this pull request May 2, 2023
jbertran added a commit that referenced this pull request May 3, 2023
tlhunter pushed a commit that referenced this pull request May 3, 2023
@jbertran jbertran mentioned this pull request May 4, 2023
jbertran added a commit that referenced this pull request May 4, 2023
jbertran added a commit that referenced this pull request May 4, 2023
jbertran added a commit that referenced this pull request May 4, 2023
jbertran added a commit that referenced this pull request May 4, 2023
jbertran added a commit that referenced this pull request May 5, 2023
jbertran added a commit that referenced this pull request May 5, 2023
This was referenced Jun 1, 2023
@amarziali amarziali mentioned this pull request Jun 22, 2023
1 task
@tlhunter tlhunter deleted the jbertran/service-naming-framework branch January 19, 2024 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants