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

feat, docs: support mongoose >=5.7.0 <7 #2547

Closed
wants to merge 2 commits into from
Closed

Conversation

trentm
Copy link
Member

@trentm trentm commented Jan 25, 2022

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.
  • Update documentation
  • Add CHANGELOG.asciidoc entry

@trentm trentm self-assigned this Jan 25, 2022
@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Jan 25, 2022
@trentm
Copy link
Member Author

trentm commented Jan 25, 2022

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
Copy link
Member Author

trentm commented Jan 25, 2022

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.

@apmmachine
Copy link
Contributor

💚 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: 2db939a

Test stats 🧪

Test Results
Failed 0
Passed 243005
Skipped 0
Total 243005

🤖 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!)

@trentm
Copy link
Member Author

trentm commented May 30, 2022

@aniketbiprojit
Copy link

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

@trentm
Copy link
Member Author

trentm commented Jan 26, 2023

@aniketbiprojit Some history:

  • The <5.7.0 for mongoose was added to that table in the documentation in docs: set upper bounds for current mongo support #1375 3 years ago, because at the time it wasn't supported.
  • Since then mongodb support was added (feat(mongodb): instrumentation #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.

@aniketbiprojit
Copy link

aniketbiprojit commented Jan 26, 2023

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();
  }
}

trentm added 2 commits June 6, 2023 09:26
Starting with mongoose 5.7.0, it uses "mongodb" (rather than
"mongodb-core") as a backend.
@trentm trentm force-pushed the trentm/mongoose-6-support branch from 2db939a to d79ba4b Compare June 6, 2023 16:39
@trentm
Copy link
Member Author

trentm commented Oct 26, 2023

This was handled in #3653

@trentm trentm closed this Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.4-candidate agent-nodejs Make available for APM Agents project planning.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants