Skip to content

Conversation

david-luna
Copy link
Member

@david-luna david-luna commented Oct 27, 2023

This PR sets span outcome for mongodb commands. The tests realised (see related issue) reveals that a failed command triggers:

  • a CommandFailed event with a property named failure
  • a CommandSucceeded event with the a property named reply. The presence of a sub-property named writeErrors indicates the command failed.

Closes: #3182

Checklist

  • Implement code
  • Add tests
  • Update TypeScript typings
  • Update documentation
  • Add CHANGELOG.asciidoc entry
  • Commit message follows commit guidelines

@github-actions github-actions bot added the agent-nodejs Make available for APM Agents project planning. label Oct 27, 2023
Copy link
Member Author

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

/test tav mongodb

@david-luna david-luna requested a review from trentm October 27, 2023 14:05
Copy link
Member Author

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

/test tav mongodb

@david-luna
Copy link
Member Author

david-luna commented Oct 27, 2023

TAV tests failing for mongo@3.5.11 on the bogus deleteOne test because it does not report error:

  • event fired is a success
  • there is no writeError in the reply

I think it was not considered an error before so:

  • should we adapt the test?
  • or should we reduce our versions to test?

@trentm
Copy link
Member

trentm commented Oct 27, 2023

I think it was not considered an error before so:

* should we adapt the test?

* or should we reduce our versions to test?

If we could find a reliable way to have a mongo client failure for testing, that would be great. I would try for a little bit to find one, but if you cannot after 5-10 minutes, then it is fine to reduce the error test to just the versions on which we know how to trigger one.

@david-luna
Copy link
Member Author

david-luna commented Oct 27, 2023

If we could find a reliable way to have a mongo client failure for testing, that would be great.

I've tried both promise and callback ways

// with Promises
try {
  await collection.deleteOne({ item: 'eggs' }, { hint: 'foo' });
  console.info('success in .deleteOne() with bogus "hint"'); // <- it call this log
} catch (err) {
  console.info('error in .deleteOne() with bogus "hint"');
}

// with callback
collection.deleteOne({ item: 'eggs' }, { hint: 'foo' }, function (err, res) {
  if (err) {
    console.log('error on callback', err);
  } else {
    console.log('no error on callback', res); // <- it call this log
  }
});

And inspecting the CommandSuccessEvent I don't see any hint

// both Promises & callbacks return the same shape
{
  "connectionId": "localhost:27017",
  "requestId": 1,
  "commandName": "delete",
  "duration": 1,
  "reply": {
    "n": 0,
    "ok": 1
  }
}

This is consistent through ALL tested versions, the difference is that 3.5.11 and below do not report error data in the CommandSucceededEvent. So I think we should go directly to the solution you proposed on the issue comment

Copy link
Member Author

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

/test tav mongodb

@david-luna david-luna requested a review from trentm October 27, 2023 23:23

// From mongodb@3.6.0 and up some commands like `deleteOne` may contain
// error data inside the `reply` property. It makes sense to capture it.
const writeErrors = event && event.reply && event.reply.writeErrors;
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I think we could use optional chaining with our new min-node version now: https://node.green/#ES2020-features-optional-chaining-operator-----
No need to. Just saying it is an option for our new code now.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO this a proper use case for optional chaining. I'll add it in the next commit. Thanks for pointing that out :)

const writeErrors = event && event.reply && event.reply.writeErrors;

if (writeErrors && writeErrors.length) {
// TODO: this gets linked to the parent transaction. How to link it to the span?
Copy link
Member

Choose a reason for hiding this comment

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

captureError has a parent option for just this case:

//   - `opts.parent` - A Transaction or Span instance to make the parent of
//     this error. If not given (undefined), then the current span or
//     transaction will be used. If `null` is given, then no span or transaction
//     will be used.

I guess the emitted CommandSucceededEvents (and the other events) are not in the context of the created span, which is fine.

Copy link
Member Author

@david-luna david-luna Oct 30, 2023

Choose a reason for hiding this comment

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

You're right, events fired run in a different context so the parent option comes in handy

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

const TEST_USE_CALLBACKS = semver.satisfies(MONGODB_VERSION, '<5');

// From mongodb@3.6.0 and up CommandSuccessEvents may contain errors
const MONGODB_SUCESS_WITH_ERRORS = semver.satisfies(MONGODB_VERSION, '>=3.6.0');
Copy link
Member

Choose a reason for hiding this comment

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

nit: typo

Suggested change
const MONGODB_SUCESS_WITH_ERRORS = semver.satisfies(MONGODB_VERSION, '>=3.6.0');
const MONGODB_SUCCESS_WITH_ERRORS = semver.satisfies(MONGODB_VERSION, '>=3.6.0');

Copy link
Member Author

Choose a reason for hiding this comment

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

At this pace I'll earn a typo master badge from github 😅

@david-luna david-luna requested a review from trentm October 30, 2023 10:26
Copy link
Member Author

@david-luna david-luna left a comment

Choose a reason for hiding this comment

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

/test tav mongodb

@david-luna david-luna merged commit bb65827 into main Oct 30, 2023
@david-luna david-luna deleted the dluna/3182-mongodb-set-span-outcome branch October 30, 2023 16:14
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agent-nodejs Make available for APM Agents project planning.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

mongodb instrumentation should set span.outcome and captureError() on commandFailed

2 participants