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

Add more context to MongoDB Span #1604

Closed
wants to merge 1 commit into from
Closed

Conversation

glucaci
Copy link
Contributor

@glucaci glucaci commented Jan 18, 2022

  • Before
    image

  • After
    image

@apmmachine
Copy link
Contributor

apmmachine commented Jan 18, 2022

❕ Build Aborted

The PR is not allowed to run in the CI yet

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

Expand to view the summary

Build stats

  • Reason: The PR is not allowed to run in the CI yet

  • Start Time: 2022-01-25T13:20:27.943+0000

  • Duration: 4 min 56 sec

  • Commit: 2a48e83

🤖 GitHub comments

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

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

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

Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

Thanks for opening this - I added 1 thing regarding Service.Resource which should be removed.

Additionally, we'd also need to adapt tests to cover this.

The rest looks good - nice catch! 👍

DnsEndPoint dnsEndPoint => new Destination { Address = dnsEndPoint.Host, Port = dnsEndPoint.Port },
DnsEndPoint dnsEndPoint => new Destination
{
Service = new Destination.DestinationService
Copy link
Contributor

Choose a reason for hiding this comment

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

Service does not need to be set here. Two things to this:

  1. According to the current spec Service.Resource should be just monodb. Spec here
  2. There is a logic in Span.cs here which fills this.

So this should be removed.

@@ -55,7 +55,7 @@ private void HandleCommandStartEvent(CommandStartedEvent @event)
var span = currentExecutionSegment.StartSpan(
@event.CommandName,
ApiConstants.TypeDb,
"mongo", isExitSpan: true);
"mongodb", isExitSpan: true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice - indeed, this should be mongodb.

@gregkalapos
Copy link
Contributor

This is done in #1677.

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