Skip to content

Conversation

@amarziali
Copy link
Contributor

What does this PR do?

This applies the service naming schema logic (see #2941) to grpc.

This PR relies on #3056 for the split into different files according to general area (messaging, storage) and a few optimizations to the service naming computation.

Motivation

Complete the service representation feature

Plugin Checklist

  • Unit tests.

Additional Notes

@amarziali amarziali requested a review from a team as a code owner June 30, 2023 15:31
@github-actions
Copy link

github-actions bot commented Jun 30, 2023

Overall package size

Self size: 4.74 MB
Deduped: 61.2 MB
No deduping: 61.25 MB

Dependency sizes

name version self size total size
@datadog/pprof 2.2.3 14.25 MB 15.13 MB
@datadog/native-iast-taint-tracking 1.5.0 14.86 MB 14.86 MB
@datadog/native-appsec 3.2.0 13.38 MB 13.39 MB
protobufjs 7.1.2 2.76 MB 6.55 MB
@datadog/native-iast-rewriter 2.0.1 2.09 MB 2.1 MB
@opentelemetry/core 1.14.0 872.87 kB 1.47 MB
@datadog/native-metrics 2.0.0 898.77 kB 1.3 MB
@opentelemetry/api 1.4.1 780.32 kB 780.32 kB
opentracing 0.14.7 194.81 kB 194.81 kB
semver 7.5.3 93.39 kB 123.79 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 Jun 30, 2023

Codecov Report

Merging #3327 (0ac5fcd) into master (6810871) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #3327      +/-   ##
==========================================
- Coverage   86.04%   85.99%   -0.05%     
==========================================
  Files         200      200              
  Lines        7787     7791       +4     
  Branches       33       33              
==========================================
  Hits         6700     6700              
- Misses       1087     1091       +4     
Impacted Files Coverage Δ
...ages/dd-trace/src/service-naming/schemas/v0/web.js 42.85% <0.00%> (-17.15%) ⬇️
...ages/dd-trace/src/service-naming/schemas/v1/web.js 42.85% <0.00%> (-17.15%) ⬇️

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

@pr-commenter
Copy link

pr-commenter bot commented Jun 30, 2023

Benchmarks

Comparing candidate commit 0ac5fcd in PR branch andrea.marziali/naming-grpc with baseline commit 6810871 in branch master.

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

Copy link
Member

@tlhunter tlhunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the molecular errors are pretty real. Might it be due to the changes to setup/mocha.js?

@amarziali amarziali force-pushed the andrea.marziali/naming-grpc branch from 178ee2e to 563d365 Compare July 3, 2023 08:34
@amarziali amarziali requested a review from tlhunter July 3, 2023 08:35
Copy link
Member

@tlhunter tlhunter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the grpc errors are pretty real. I've been thrown off by the withNamingSchema() usage of done before as well.

     TypeError: done is not a function
      at Object.callback (packages/datadog-plugin-grpc/test/server.spec.js:87:66)

@amarziali amarziali requested a review from tlhunter July 3, 2023 16:54
@amarziali amarziali merged commit 1b42c04 into master Jul 3, 2023
tlhunter pushed a commit that referenced this pull request Jul 5, 2023
* grpc: service representation naming

* fix tests
@tlhunter tlhunter mentioned this pull request Jul 5, 2023
tlhunter pushed a commit that referenced this pull request Jul 5, 2023
* grpc: service representation naming

* fix tests
tlhunter pushed a commit that referenced this pull request Jul 5, 2023
* grpc: service representation naming

* fix tests
This was referenced Jul 5, 2023
tlhunter pushed a commit that referenced this pull request Jul 5, 2023
* grpc: service representation naming

* fix tests
tlhunter added a commit that referenced this pull request Jul 5, 2023
tlhunter added a commit that referenced this pull request Jul 5, 2023
tlhunter pushed a commit that referenced this pull request Jul 6, 2023
* grpc: service representation naming

* fix tests
tlhunter pushed a commit that referenced this pull request Jul 6, 2023
* grpc: service representation naming

* fix tests
tlhunter pushed a commit that referenced this pull request Jul 6, 2023
* grpc: service representation naming

* fix tests
.then(done)
.catch(done)
spanProducerFn()
spanProducerFn(done)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug? When the .then(done) above calls, it'll execute the done() function. However, when spanProducerFn(done) calls, it will also run done(). Running this function twice results in a test failure. Was it added accidentally?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless it's explicitly used for the purpose of failing tests and should always have a truthy argument?

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.

3 participants