apm-agent-nodejs icon indicating copy to clipboard operation
apm-agent-nodejs copied to clipboard

feat, docs: support mongoose >=5.7.0 <7

Open trentm opened this issue 3 years ago • 4 comments

Starting with mongoose 5.7.0, it uses "mongodb" (rather than "mongodb-core") as a backend.

Checklist

  • [ ] Figure out why no ".find" spans with mongoose >=5.7.0 (see comment below).
  • [ ] Decide if we want to have explicit mongoose tests. Currently we rely on the mongodb-core tests, which I think should still suffice.
  • [x] Update documentation
  • [ ] Add CHANGELOG.asciidoc entry

trentm avatar Jan 25 '22 21:01 trentm

Using the examples/trace-mongoose.js in this starter PR I don't get a mongodb span that I expect for a Person.find(...).

For example with [email protected] I see:

% node examples/trace-mongoose.js
...

trace 6b6b2e
`- transaction 7873ce "t1" (65.387ms, outcome=unknown)
   `- span f6a83c "example-trace-mongoose.people.insert" (6.506ms)
   `- span baf633 "example-trace-mongoose.people.find" (0.287ms, sync)
   `- span e8dce2 "admin.$cmd.command" (2.702ms)

However with [email protected]:

% node examples/trace-mongoose.js
...

    trace fa96aa
    `- transaction e23f5c "t1" (64.876ms, outcome=unknown)
       `- span 316ebf "example-trace-mongoose.people.insert" (5.91ms)
       `- span 5d36de "admin.$cmd.endSessions" (1.7095ms)

And with mongoose@6:

% npm ls mongoose
[email protected] /Users/trentm/el/apm-agent-nodejs6
└── [email protected]

% node examples/trace-mongoose.js
...

    trace 26f0e3
    `- transaction dd7638 "t1" (70.447ms, outcome=unknown)
       `- span bbec6d "example-trace-mongoose.people.create" (4ms, sync)
       `- span 2de735 "example-trace-mongoose.people.insert" (2ms, sync)
       `- span 2254d3 "admin.$cmd.endSessions" (0ms, sync)

Where is the ".find" span? Is there a bug in our instrumentation or is this some caching thing in mongoose?

trentm avatar Jan 25 '22 21:01 trentm

Ah, actually, if I change the script above to:

  1. Not delete all entries at the end (so that the DB has one or more Person entries), then
  2. Remove the block that adds the "bill" entry, then
  3. Re-run.

Now I get a .find span:

    trace 9d9279
    `- transaction 4ff4c9 "t1" (66.033ms, outcome=unknown)
       `- span 2498be "example-trace-mongoose.people.create" (3ms, sync)
       `- span c8c237 "example-trace-mongoose.people.find" (2ms, sync)
       `- span 593382 "admin.$cmd.endSessions" (0ms, sync)

I don't think it is the Mongoose client caching a write and avoiding hitting the DB. Perhaps it is some pipelining or bulk query or transaction where that ".insert" is actually doing the find query as well? Seems unlikely.

There is still something going on in the DB client I don't understand.

trentm avatar Jan 25 '22 21:01 trentm

:green_heart: Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-25T21:25:58.205+0000

  • Duration: 21 min 58 sec

  • Commit: 2db939aa372df3a0c67236feff3beffc1e631ffd

Test stats :test_tube:

Test Results
Failed 0
Passed 243005
Skipped 0
Total 243005

:robot: GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run module tests for <modules> : Run TAV tests for one or more modules, where <modules> can be either a comma separated list of modules (e.g. memcached,redis) or the string literal ALL to test all modules

  • run benchmark tests : Run the benchmark test only.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

apmmachine avatar Jan 25 '22 21:01 apmmachine

User request for this at https://discuss.elastic.co/t/support-for-mongoose-version-5-7/305849

trentm avatar May 30 '22 23:05 trentm

I am confused why mongoose@^6.7.2 isn't supported. I am using 6.8.3 and it internally uses [email protected] which should be supported as per this.

Is there a flag or something that needs to be enabled?

image

aniketbiprojit avatar Jan 26 '23 20:01 aniketbiprojit

@aniketbiprojit Some history:

  • The <5.7.0 for mongoose was added to that table in the documentation in https://github.com/elastic/apm-agent-nodejs/pull/1375 3 years ago, because at the time it wasn't supported.
  • Since then mongodb support was added (https://github.com/elastic/apm-agent-nodejs/pull/1423) which should mean later mongoose versions are supported. At the time the documentation for the supported versions of mongoose were not updated.
  • More recently I opened this starter PR to look at bumping that supported version range in the documentation and added an example script to actually test mongoose usage -- because we currently don't directly test mongoose. I ran into some things I don't yet understand (see above comments) and haven't yet had a chance to come back to this.

It looks to me like instrumentation of mongoose versions after v5.7.0 is working, at least partially.

trentm avatar Jan 26 '23 21:01 trentm

I am not seeing any instrumentation response in requests/trace from mongoose@ 6.8.3 ([email protected]) on APM.

I had to create a plugin to use pre and post hooks for meanwhile but this doesn't look like methodology. Also, worried about how this effects the cost of each transaction.

targetSchema.pre(/.*/, preQueryHook);
targetSchema.post(/.*/, postQueryHook);

function preQueryHook() {
  if (this.op === undefined) {
    return;
  }
  this.__span = apmAgent.startSpan(
    `${target.op} on ${target._collection?.collectionName
    } with filter: ${JSON.stringify(
      target._conditions ?? {},
      undefined,
      4,
    )}`,
  );
  const span = this.__span as apm.Span | undefined;
  if (span) {
    span.addLabels({
      filter: JSON.stringify(target._conditions ?? {}, undefined, 4),
      update: JSON.stringify(target._update ?? {}, undefined, 4),
      projection: JSON.stringify(
        target._userProvidedFields ?? {}
      ),
    });

    span.setType('db', 'MongoDB', target.op);
  }
}


function postQueryHook() {
  if (this.__span) {
    this.__span.end();
  }
}

aniketbiprojit avatar Jan 26 '23 22:01 aniketbiprojit

This was handled in https://github.com/elastic/apm-agent-nodejs/pull/3653

trentm avatar Oct 26 '23 17:10 trentm